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

support opt-in rules #345

Merged
merged 2 commits into from
Jan 12, 2016
Merged

support opt-in rules #345

merged 2 commits into from
Jan 12, 2016

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Jan 12, 2016

Closes #256 /cc @keith

We could rules conform to this that are either not universally accepted, or applicable (e.g. #245 & #321) and rules that have too high of a false positive rate (e.g. #201 & #206).

@keith
Copy link
Collaborator

keith commented Jan 12, 2016

Awesome! 👍

jpsim added a commit that referenced this pull request Jan 12, 2016
@jpsim jpsim merged commit b9a13be into master Jan 12, 2016
@jpsim jpsim deleted the jp-opt-in branch January 12, 2016 22:29
@daniel-beard
Copy link
Contributor

This is great! So enabled_rules: in the yml file acts as a whitelist?

self.rules = rules.filter { rule in
let id = rule.dynamicType.description.identifier
if validDisabledRules.contains(id) { return false }
return enabledRules.contains(id) || !(rule is OptInRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to have the OptInRule or possibly a different mechanism for it. It makes enablesRules only useful for opting in to non-default rules, rather a definitive whitelist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. That's not what I was going for, and not what I outlined in #256 (comment).

What you want is a definitive list of rules to run, ignoring enabled_rules and disabled_rules, which now reading back that comment I linked to, was the motivation for a rules config option.

I see value in that, but it does have to be weighed against the complexity of having a white list, a black list, and a list list 😞 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it, although I got a different meaning from @scottrhoyt 's comment #256 (comment) which seems to imply either a white list or black list, but not both (or opt in).

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