Explicitly define enabled flake8 errors and warnings

[ShadowJonathan]

Closes #9366 either way or the other.

I think #9366 has existed for quite some time now, the last action being to enable B90, a set of opinionated lints.

However, this was blocked by having all flake8 lints be explicitly defined (using select=), as there is no way to "add" a lint code to check. B90 being optional required this.

This PR could unblock that effort, but I'd want to petition this anyway, as I think now it'd be a good time to look at this again with fresh eyes.

If this PR goes through, I might make an issue about adding B90, discussing the pros and cons of those lints.

---

The advantages of doing this would be;

• Having a clear set of lints (as flake8 defaults are hard to find[1]), which could help resolve "is this enabled or not?" questions when adding flake8 plugins in the future.
• Being able to add optional lints (such as B90) easily.

Disadvantages would be;

• Flake8 can change its default set of lints inbetween versions, and this would override it.
  • Note that this would only apply if they'd be adding a whole new category, currently it just selects E, W, and F, which are all already the categories flake8 operate in. Any new lints would most likely be added to those, and automatically included here.

[1]: Currently the only way to find flake8 defaults are to run flake8 with verbose on, and watching a line where it tells about overriding a set of defaults. Here I copied that list as it was per flake8 4.0.1.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

[richvdh]

I still don't really understand what this is actually changing.

[ShadowJonathan]

This is not changing any config to a different behaviour, but it is taking existing implicit configuration, and turning it into something explicit.

It's needed to add something like Bugbear's B90 codes, see here under "opinionated warnings"

Furthermore, it is also helpful when asking questions like "which flake8 lints are actually enabled?", as the flake8 site does not elaborate on this, and someone would have to;

• set this key in this config to a value (any value)
• run flake8 with verbose on
• hunt a log-line where it says it is overwriting a default set of lints, and grab the ones from there

---

This is not changing anything, but improving on two fronts;

• Allowing flake8 opt-in rules to be easily added
  • This step (of explicitly defining default lines) would have to be done regardless if we wanna add any opt-in lints in the future
• Reduce debugging steps of "why isn't this lint enabled/running?"

[DMRobertson]

the last action being to enable B90, a set of opinionated lints.

However, this was blocked by having all flake8 lints be explicitly defined (using select=), as there is no way to "add" a lint code to check. B90 being optional required this.

Does --extend-select not suffice for this?

[DMRobertson]

I don't see why this is useful.

• We're not spending our time updating the config often, writing new lints for flake8 ourselves; we trust flake8 to provide sensible defaults.
• As of Lock Dependencies in Synapse #11537, a fixed version of flake8 is pinned for use in CI, so we are running the same set of defaults and hence the same set of lints everywhere.
• Turning on any additional checks (e.g. the B90x series of lints you mention) should be perfectly possible using the extend-select config option as mentioned here.

Additionally, AFAICS the proposed change is identical to #9370, which we closed last year. I am minded to close this too.

[ShadowJonathan]

Does --extend-select not suffice for this?

...it does, it seems to have been added very recently. This kinda defeats the point of the PR.

I'll close this, in the future I might add B90 lints under extend-select.