Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section in .sonarrc to ignore responses from certain domains #179

Closed
wants to merge 2 commits into from

Conversation

sarvaje
Copy link
Contributor

@sarvaje sarvaje commented May 5, 2017

Fix #108

@molant
Copy link
Member

molant commented May 5, 2017

This feature is for the users to say "I don't want you to run any rules in these domains". This means that it can't be handled at the rule level but rather sonar.ts, probably when we are about to emit an event.
What we could do is:

@sarvaje
Copy link
Contributor Author

sarvaje commented May 6, 2017

@molant I've changed how we check if the url is ignored or not.

I'm adding the label work-in-progress because I would like to add a couple more tests

src/lib/sonar.ts Outdated
import * as browserslist from 'browserslist';
import { EventEmitter2 as EventEmitter } from 'eventemitter2';
import * as url from 'url';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Built-in packages go on the top.

src/lib/sonar.ts Outdated
let ignored = false;

if (urlsIgnored) {
for (const urlIgnored of urlsIgnored) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use 'some()' instead of for of and break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a Set, there is no 'some()' method. Do you want me to change to an Array?

src/lib/sonar.ts Outdated
const ignoredUrls = this.ignoredUrls.get('all');

if (ignoredUrls) {
for (const ignoredUrlGlobal of ignoredUrls) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 'some' and if found then return promise.

@sarvaje
Copy link
Contributor Author

sarvaje commented May 8, 2017

@molant @alrra this is ready to review and merge if everything is ok

Copy link
Contributor

@alrra alrra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged with some minor modifications.

@@ -7,10 +7,11 @@
// Requirements
// ------------------------------------------------------------------------------

import { EventEmitter2 as EventEmitter } from 'eventemitter2';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventEmitter2 should be in the second group.

(I'll fix it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants