Adds allow_list_rules setting for configs.

[lisroach]

Summary

Adds a new setting allow_list_rules that enables setting specific rules as "on". This solves a couple things:

• Safer upgrades of Fixit versions for users because newly added rules can be turned off by default
• Along with inheritable configs (to be implemented Discussion: Fixit Config File Requirements #183) this gives finer grain control over what rules run in individual directories

If allow_list_rules is left out of a config or set to [] it defaults to allow all to keep backwards compatibility.

Test Plan

Added new unit tests. Also updated the docs.

[lisroach]

tox -e py38
tox -e py37
tox -e autofix
pyre --preserve-pythonpath check

All pass locally. Are the issues unrelated here?

[thatch]

@lisroach the windows failures are because of the multiline strings with escaped newlines. It's using the git mode that checks it out with dos newlines, so instead of <backslash><LF> it is <backslash><CR><LF>, and it's complaining that <backslash><CR> is an invalid escape. I verified that it does not do the right thing when run, either. (It seems to omit the CR, but leave the subsequent LF.)

I think elsewhere in fixit and libcst they use textwrap.dedent and just use an indented multiline string. I think blank lines are allowed in yaml, so could just omit the backslash too.

[codecov-commenter]

Codecov Report

Merging #184 (b71cceb) into master (cb1d589) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
+ Coverage   86.03%   86.19%   +0.15%     
==========================================
  Files          91       91              
  Lines        3703     3744      +41     
==========================================
+ Hits         3186     3227      +41     
  Misses        517      517              

Impacted Files	Coverage Δ
fixit/common/base.py	94.66% <100.00%> (+0.07%)	⬆️
fixit/common/config.py	91.04% <100.00%> (ø)
...mmon/tests/dummy_package/dummy_subpackage/dummy.py	100.00% <100.00%> (ø)
fixit/common/tests/test_imports.py	100.00% <100.00%> (ø)
fixit/common/utils.py	92.10% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb1d589...b71cceb. Read the comment docs.

[lisroach]

Thanks @thatch! That helped a lot.

The Pyre error is unrelated, from the latest LibCST upgrade based on Jimmy's comments from #182. I'll work on a PR to fix that bug.

[lisroach]

#187 for the Pyre error

[lisroach]

#190 for the autofix errors.

[williamlw999-fb]

I'm not sure if block_list_rules should always take precedence when it comes to inheritance. Say a new lint is being rolled out that has a high impact area. A team may want to turn it off by default for their directory but enable it gradually in subdirectories. I think that as part of the inheritance, it may make sense to override the block / allow list in tandem. That is, each key / rule should only exist in at most one of those lists. Thus, when a sub-directory's config allow lists a previously block listed rule, the inheritance should recognize and remove the pre-existing block on the rule.

[lisroach]

@williamlw999-fb interesting point! My main reasoning behind the ability for block_list_rules to take precedence is in the case of a dangerous rule. If we discover a rule is dangerous it can be turned off for an entire directory in one swoop, vs having to track down all the allow_list_rules that might contain it and turn it off individually.

One thing I think you might be misunderstanding is that a rule is not on by default if it does not exist in the block_list_rules. So if a parent config does not need to add a new rule to the block_list_rules in order to turn it off by default as long as they have other rules in the allow_list_rules.

So if a parent config looks like:

block_list_rules: []
allow_list_rules: [MyRule1]

and a new rule named YourRule comes out, YourRule would not be on for the subdirs. It is only on if it is added to allow_list_rules. So slowly rolling out a rule to subdirs should be as simple as adding to the subdirs' allow_list_rules.

The one thing I don't like about this is if you have:

block_list_rules: [MyRule1]
allow_list_rules: []

and you roll out YourRule, in this case YourRule is on everywhere. I would like to require allow_list_rules to always specify all the rules that are on, but in order to keep backwards compatibility we need to support allow_list_rules empty defaulting to everything on.

[lisroach]

I'm going to merge this PR to unblock myself, I am happy to have more discussion on this and we can make changes before the next FixIt release if the current way isn't working.