Inheritable configs

[lisroach]

Summary

Putting up a draft to start discussion. This implements what was posted in #183. Configs are inheritable via simple hierarchy. There is nothing fancy like custom schema definitions or partial inheritance. A config will be inheritable if they add the key inherit = True, it defaults to False if the key is not set.

Different keys are inherited differently, see fixit/common/config.schema.json for a breakdown and jsonmerge for more details.

allow_list_rules is kind of a funny one, since if it is not included it defaults to all rules being allowed. If a leaf node inherits an empty allow_list_rules but the leaf config defines rules in the list, it has the effect of turning off all the rules that are not listed.

rule_configs currently use jsonmerge's objectMerge with the defaults set. This means if there are two identical keys with different values, the value of the leaf config overwrites the value of the root. If keys are not identical, they are both included in the final result. For example:

root.fixit.config.yaml

"rule_config": {
    "UnusedImportsRule": {
        "ignored_unused_modules": [
            "__future__",
            "__static__",
            "__static__.compiler_flags",
            "__strict__",
        ]
    },
   "Other Rule": { "yes": True}
}

leaf.fixit.config.yaml

"rule_config": {
   "UnusedImportsRule": {
        "ignored_unused_modules": ["__init__"]
  },
  "CoolRule": {"was": "sup"}
}

Results in:

"rule_config": {
   "UnusedImportsRule": {
        "ignored_unused_modules": ["__init__"]
  },
  "Other Rule": { "yes": True},
  "CoolRule": {"was": "sup"}

As expressed in the discussion post, I think this is most often the state that we want. But rules are variable, and I can forsee cases where someone would want this different (merging of lists instead of overwriting, for example, or completely overwriting). This seems like a good starting point for today.

Test Plan

Added unit tests

[codecov-commenter]

Codecov Report

Merging #198 (9a4a6fa) into master (ad655b2) will increase coverage by 0.48%. The diff coverage is 98.97%.

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   86.19%   86.67%   +0.48%     
==========================================
  Files          91       93       +2     
  Lines        3744     3896     +152     
==========================================
+ Hits         3227     3377     +150     
- Misses        517      519       +2     

Impacted Files	Coverage Δ
fixit/common/utils.py	91.59% <83.33%> (-0.51%)	:arrow_down:
fixit/common/config.py	92.63% <97.36%> (+1.58%)	:arrow_up:
fixit/common/base.py	94.73% <100.00%> (+0.07%)	:arrow_up:
fixit/common/tests/dummy_package/dummy_1.py	100.00% <100.00%> (ø)
...mmon/tests/dummy_package/dummy_subpackage/dummy.py	100.00% <100.00%> (ø)
...it/common/tests/duplicate_dummy/duplicate_dummy.py	100.00% <100.00%> (ø)
...icate_dummy/subduplicate_dummy/duplicate_dummy1.py	100.00% <100.00%> (ø)
fixit/common/tests/test_config.py	100.00% <100.00%> (ø)
fixit/common/tests/test_imports.py	100.00% <100.00%> (ø)
... and 1 more

---

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 ad655b2...9a4a6fa. Read the comment docs.

[amyreese]

More generally, is this expecting to up-front merge all configs, and then apply the same merged configs to all paths being linted? Or is this attempting to only find the hierarchy of configs relevant to each individual file, merge those configs, and repeat that find/merge individually for every single file being linted?

I would expect the latter to be the more "obvious/expected" implementation, but it looks like you're doing the former? If you're working on the former, and two subdirectories want to have two different sets of rules enabled or disabled, how is that conflict of desires being dealt with?

[lisroach]

is this expecting to up-front merge all configs, and then apply the same merged configs to all paths being linted?

No, that is not my intent.

presence of a config in root/a/b.yaml would influence linting on root/d/f.py

This also should not be the case, it walks up from the cwd to find all configs above, it should not walk back down a branch.

But these question do make me think about the current implementation. Right now, FixIt needs to be run from the bottom-most directory with a config file for that leaf config to be applied to all the files in its directory. I think if you run FixIt from a higher up directory it would miss the leaf configs. That isn't ideal, so I am taking a look now at the config file generation and how paths are found to be linted.

[amyreese]

Hey there! We appreciate your contributions, but we're in the process of making some large changes to the core of Fixit. We will try to have more info about the direction we're heading soon, but in the mean time, we are closing all outstanding PR's from before we started this work. Thank you for your understanding.