config validation?

[ivirshup]

This is a feature request for more strict config validation.

Here is the issue that inspired this feature request: scverse/anndata#540

A space managed to be placed in front of the per-file-ignore line in our .flake8 config. This resulted in it being parsed as just another entry in ignore, and all elements in the following lines split by white space and added parsed as entries in ignore. Practically, this resulted in:

This config

# Can't yet be moved to the pyproject.toml due to https://github.com/PyCQA/flake8/issues/234
[flake8]
max-line-length = 88
ignore = # module imported but unused -> required for Scanpys API
         F401,
         # line break before a binary operator -> black does not adhere to PEP8
         W503,
         # line break occured after a binary operator -> black does not adhere to PEP8
         W504,
         # line too long -> we accept long comment lines; black gets rid of long code lines
         E501,
         # whitespace before : -> black does not adhere to PEP8
         E203,
         # missing whitespace after ,', ';', or ':' -> black does not adhere to PEP8
         E231,
         # module level import not at top of file -> required to circumvent circular imports for Scanpys API
         E402,
         # continuation line over-indented for hanging indent -> black does not adhere to PEP8
         E126,
         # E266 too many leading '#' for block comment -> Scanpy allows them for comments into sections
         E262,
         # inline comment should start with '# ' -> Scanpy allows them for specific explanations
         E266,
         # Do not assign a lambda expression, use a def -> Scanpy allows lambda expression assignments,
         E731,
         # allow I, O, l as variable names -> I is the identity matrix, i, j, k, l is reasonable indexing notation
         E741
per-file-ignores =
    # F811 Redefinition of unused name from line, does not play nice with pytest fixtures
    tests/test*.py: F811
    # F821 Undefined name, can't import AnnData or it'd be a circular import
    anndata/compat/_overloaded_dict.py: F821
    # E721 comparing types, but we specifically are checking that we aren't getting subtypes (views)
    anndata/tests/test_readwrite.py: E721
exclude =
    .git,
    __pycache__,
    build,
    docs/_build,
    dist,

>>> from flake8.api.legacy import get_style_guide
>>> get_style_guide().options.ignore

Parsing to this:

['#',
 'module',
 'imported',
 'but',
 'unused',
 '->',
 'required',
 'for',
 'Scanpys',
 'API',
 'F401',
 'W503',
 'W504',
 'E501',
 'E203',
 'E231',
 'E402',
 'E126',
 'E262',
 'E266',
 'E731',
 'E741',
 'per-file-ignores',
 '=',
 'tests/test*.py:',
 'F811',
 'anndata/compat/_overloaded_dict.py:',
 'F821',
 'anndata/tests/test_readwrite.py:',
 'E721']

We didn't immediately notice because adding per-file-ignores still made errors go away. Unfortunately it was making them go away globally.

I would have expected this to throw an error at some point. This could have happened from invalid entries ending up in the ignore table, or (with a more structured config format) errors from levels of indentation varying so widely.

I expect pyproject.toml (#234) support would have solved this particular issue since it would have been a parse error.

---

Please describe how you installed Flake8

pip, though this has also been installed as part of a pre-commit hook

---

*Please provide the exact, unmodified output of `flake8 --bug-report`*

{
  "dependencies": [],
  "platform": {
    "python_implementation": "CPython",
    "python_version": "3.8.6",
    "system": "Darwin"
  },
  "plugins": [
    {
      "is_local": false,
      "plugin": "mccabe",
      "version": "0.6.1"
    },
    {
      "is_local": false,
      "plugin": "pycodestyle",
      "version": "2.7.0"
    },
    {
      "is_local": false,
      "plugin": "pyflakes",
      "version": "2.3.1"
    }
  ],
  "version": "3.9.0"
}

[asottile]

seems pretty unfortunate, but had it been exclude instead of ignore there would not have been a way to detect this as invalid!

the tough thing about this is if the configuration would have been added incrementally you would have found this -- it seems that it was a bad copy paste error that led to this (the same sort of argument applies to test-driven-development)

I'm not sure that there's much that can be done here without potentially breaking some existing use cases as error codes don't really have a restriction even though conventionally they have been 1-3 letters followed by 3 numbers (they can technically be any string!) -- maybe it's fine to break these users?

[asottile]

also enforcing that would break your config for another reason (as shown from the first part of the parsed value) so I think this is not really actionable without breaking a lot of people

so I'm going to decline this at this time

[ivirshup]

also enforcing that would break your config for another reason (as shown from the first part of the parsed value)

I also intend to change this. Definitely had not realized it was being parsed this way, and would have preferred this to error.

Ultimately, I think a fair amount of fault here lies with a permissive config format. I totally understand this being basically unfixable for backwards compatibility, externally developed plugins, and other tools reading these entries.

I think a partial remedy here would be to increase user visibility into what options flake8 is actually applying at runtime. I tried flake8.api.legacy.get_style_guide more on a hunch after googling than because it was clearly the "blessed" way to view runtime configuration values. It would be great if there was a blessed way (maybe from the cli?) to see these values. It looks like this information is included on the second level of verbosity, but also that level is extremely verbose.

the tough thing about this is if the configuration would have been added incrementally you would have found this -- it seems that it was a bad copy paste error that led to this (the same sort of argument applies to test-driven-development)

This is a bit pedantic, but I think the incremental addition actually hurt here. Initially, we only added per files ignores after fixing the report everywhere else. There, it was indistinguishable that the per-file report was actually masking everything because there were no other cases to report. When it was added all at once, I started noticing obvious cases where flake8 should be complaining, but wasn't.

[asottile]

It would be great if there was a blessed way (maybe from the cli?) to see these values.

-vv has this information