Discussion: Fixit Config File Requirements about fixit

[lisroach]

To get things moving here I'll work on implementing allow_list_rules and see if there are any objections on the PR

[lisroach]

While waiting on review of #184, I'll put out my plan for what I think is the right design for inheritable configs (open to suggestions!).

TL;DR on the post above, I propose:

• We create inheritable configs
• The configs are inheritable via simple hierarchy
  • leaf configs merge with or override root configs
• We are selective with how each section of a config is inherited
  • Certain keys are merged the leaf configs, while others are directly overridden

I am also interested in investigating the partial configs as well, but I believe that can be evaluated later.

For implementation, I evaluated a few different libraries. I'll go into detail into those below in case people want to read into it, I sorted them from least viable to most viable. But I'll give another TL;DR here:

I believe using jsonmerge is the best option. It gives us the flexibility to configure how each section of the config should be overridden, and it should be simple to implement.

The steps to enable are basically:

1. Create a JSON Schema for the existing Config file
2. Create a directory crawler that uses jsonmerge to merge configs in each directory
3. Update the JSON Schema with the merging techniques for each key

And that's it. I am interested in anyone's thoughts (especially @jimmylai :)) and happy to change course if there are objections.

---

Library Evaluations

PyYAML Root Key Merging

PyYAML and ruamel.yaml provide a very basic inheritance functionality that allows usage of defaults defined in one config to be used elsewhere. It does this via key [merging)(https://yaml.org/type/merge.html). This proposal is essentially that there is one root config and it is the only config any file can inherit from. We would define all our defaults there and users would use their configs to add to these.

Documentation is pretty sparse here, ruyamel appears to be better documented and more up to date, but functionality looks relatively the same.

Rudimentary example:
root/config.yaml

root: &default
    block_list_patterns:
    - '@generated'
    - '@nolint'
   
subdir:
    <<: *default
    block_list_patterns:
    - '@test'

becomes

block_list_patterns:
- '@generated'
- '@nolint'
- '@test'

This is not very configurable from a users perspective, and based on the few docs I’ve found I’m not sure if you can use the inheritance across config files at all. I bring it up in case people do want to discuss the option of having only one config that is possible to inherit from, but I do not find the idea compelling.

Includes and Interpolation

This is what I think we can use for embedding partial configs for the rule_config, I also evaluated it for use as the whole thing. Instead of true inheritance, we could use includes or interpolation to share config information. This requires some more work on the users’ part as they need to manually add the correct values to their config, but it provides a good deal of flexibility.

Using includes embeds another config file into the config file that uses it, support comes from a library called pyyaml-include. An example:

root/config.yaml

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule

root/myproject/config.yaml

root_config: !include root/config.yaml
packages:
- myproject.rules

becomes...

root_config:
  block_list_patterns:
  - '@generated'
  - '@nolint'
  block_list_rules:
  - BlockListedRule
packages:
- myproject.rules

Interpolation support exists in a few libraries, including hiyapyco which I will talk about more in the next section. An example:
root/config.yaml

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule

root/myproject/config.yaml

{{ block_list_patterns }}
{{ block_list_rules }}
packages:
- myproject.rules

becomes...

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
packages:
- myproject.rules

I think these ideas are interesting, but don’t fit our entire use-case well enough on their own. Going back to my experience with Flake8, users assume there will be inheritance, and may omit entirely the needed configuration sections from the root config. There also isn’t an easy way to override or add to individual keys.

There might be some potential use for this, though. A concern I have about the single root config is that it will become bloated with a huge rule_config section. We may be able to use one of these tools to define rule_configs in their own files and pull them into configs (or the root config) in a way that will result in a much shorter file.

Merge Configs via Hierarchy

With this technique all config files in the directory are merged together into one using a merge (optionally a deep merge). There are a number of libraries that support this, like himland hiyapyco. hiyapyco is more configurable, so I’ll discuss it here.

A list of paths is passed to the tool and merged with latest path taking precedence over older path. There are two supported configuration options, METHOD_SIMPLE which replaces older config objects, and METHOD_MERGE which performs a deep merge. METHOD_MERGE is closest to what we need, so I have shown examples here:

root/config.yaml

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
fixture_dir: ./tests/fixtures
formatter:
- black
- '-'
packages:
- fixit.rules
repo_root: .
rule_config:
    ImportConstraintsRule:
        fixit:
            rules: [["*", "allow"]]

root/myproject/config.yaml

block_list_rules:
- OtherBlockListedRule
packages:
- myproject.rules
rule_config:
    ImportConstraintsRule:
        mysubdir:
            rules: [
                ["fixit", "allow"],
                ["other_module", "deny"],
                ["*", "deny"]
            ]
            ignore_tests: True
            ignore_types: True
            message: "'{imported}' cannot be imported from within '{current_file}'."

Merged config with METHOD_MERGE...

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
- OtherBlockListedRule
fixture_dir: ./tests/fixtures
formatter:
- black
- '-'
packages:
- fixit.rules
- myproject.rules
repo_root: .
rule_config:
  ImportConstraintsRule:
    fixit:
    - - '*'
      - allow
    mysubdir:
      rules:
      - - fixit
        - allow
      - - other_module
        - deny
      - - '*'
        - deny
      ignore_tests: true
      ignore_types: true
      message: '''{imported}'' cannot be imported from within ''{current_file}''.'

This works well, as we were able to merge the common items into one larger config.

But there is an issue if a leaf config attempts to override the root config:

root/myproject/config.yaml

block_list_rules:
- OtherBlockListedRule
packages:
- myproject.rules
rule_config:
    ImportConstraintsRule:
        fixit:
            rules: [
                ["fixit", "allow"],
                ["other_module", "deny"],
                ["*", "deny"]
            ]
            ignore_tests: True
            ignore_types: True
            message: "'{imported}' cannot be imported from within '{current_file}'."

Merged with config METHOD_MERGE...

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
- OtherBlockListedRule
fixture_dir: ./tests/fixtures
formatter:
- black
- '-'
packages:
- fixit.rules
- myproject.rules
repo_root: .
rule_config:
  ImportConstraintsRule:
    fixit:
      rules:
      - - '*'
        - allow
      - - fixit
        - allow
      - - other_module
        - deny
      - - '*'
        - deny
      ignore_tests: true
      ignore_types: true
      message: '''{imported}'' cannot be imported from within ''{current_file}''.'

The rule_config rules section has been appended, resulting in an invalid rule: there are two * rules. This will raise an exception, as I discussed in my first post. Because of this, I do not think we can use this technique without breaking some existing rules.

jsonmerge + custom code

This has led me to jsonmerge. Jsonmerge can better handle some of the issues that the hierarchical merge tools can’t do, like merging different keys in different ways. For example, we may want to always append to block_list_patterns but always override rule_config configurations. With jsonmerge we’d provide our own JSON schema which will define the merge strategies we want per key. This can give us a result that looks like this:

root/config.yaml

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
fixture_dir: ./tests/fixtures
formatter:
- black
- '-'
packages:
- fixit.rules
repo_root: .
rule_config:
    ImportConstraintsRule:
        fixit:
            rules: [["*", "allow"]]

root/myproject/config.yaml

block_list_rules:
- OtherBlockListedRule
packages:
- myproject.rules
rule_config:
    ImportConstraintsRule:
        fixit:
            rules: [
                ["fixit", "allow"],
                ["other_module", "deny"],
                ["*", "deny"]
            ]
            ignore_tests: True
            ignore_types: True
            message: "'{imported}' cannot be imported from within '{current_file}'."

merged config...

block_list_patterns:
- '@generated'
- '@nolint'
block_list_rules:
- BlockListedRule
- OtherBlockListedRule
fixture_dir: ./tests/fixtures
formatter:
- black
- '-'
packages:
- fixit.rules
- myproject.rules
repo_root: .
rule_config:
    ImportConstraintsRule:
        fixit:
            rules: [
                ["fixit", "allow"],
                ["other_module", "deny"],
                ["*", "deny"]
            ]
            ignore_tests: True
            ignore_types: True
            message: "'{imported}' cannot be imported from within '{current_file}'."

This is correct, and looks to be easy to implement. Another benefit of using jsonmerge is we have the potential for rules to define their own custom JSON Schemas. This gives rule creators granular control over how their rule is inherited, instead of us deciding one single solution for all rules. This is harder to implement and maintain, plus it is added complexity for rule owners, but may be a nice future feature.

[amyreese]

Most of this discussion is being handled with the new configuration planned as part of FP1: #236