Add flake8-bugbear to the test suite, resolve resulting lint errors

[ShadowJonathan]

The scope of this PR is the following;

• adding flake8-bugbear to the lint extras
• fixing all resulting flake8 errors
• adding various small fixes in the name of explicitness; (discussed in Add 'raise X from e' in more places #9318)
  • raise X from e
  • Failure(e)

PR contains some comments on areas where wider intention isn't (as) clear.

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.
• Pull request includes a sign off
• Code style is correct (run the linters)

Closes #9279

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

[ShadowJonathan]

Left some comments where I need some input.

[ShadowJonathan]

(Post-PR) Per #9279 I also mentioned B90, which is a subclass of "opinionated" linting violations from bugbear, as of commit 3ef7c6c, the following violations are reported;

synapse/rest/media/v1/_base.py:289:1: B903 Data class should either be immutable or use __slots__ to save memory. Use collections.namedtuple to generate an immutable class, or enumerate the attributes in a __slot__ declaration in the class to leave attributes mutable.
synapse/config/appservice.py:38:33: B902 Invalid first argument 'cls' used for instance method. Use the canonical first argument name in methods, i.e. self.
synapse/config/api.py:35:33: B902 Invalid first argument 'cls' used for instance method. Use the canonical first argument name in methods, i.e. self.
synapse/config/tracer.py:50:33: B902 Invalid first argument 'cls' used for instance method. Use the canonical first argument name in methods, i.e. self.
synapse/config/_base.py:34:1: B903 Data class should either be immutable or use __slots__ to save memory. Use collections.namedtuple to generate an immutable class, or enumerate the attributes in a __slot__ declaration in the class to leave attributes mutable.
synapse/replication/http/_base.py:129:36: B902 Invalid first argument **kwargs used for instance method. Use the canonical first argument name in methods, i.e. self.
tests/test_visibility.py:147:9: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/test_visibility.py:173:9: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/test_visibility.py:196:9: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/storage/test_state.py:77:9: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/test_utils/html_parsers.py:52:15: B902 Invalid first argument '_' used for instance method. Use the canonical first argument name in methods, i.e. self.
tests/rest/media/v1/test_url_preview.py:97:17: B902 Invalid first argument '_self' used for instance method. Use the canonical first argument name in methods, i.e. self.
tests/rest/client/test_transactions.py:49:13: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/util/caches/test_descriptors.py:231:21: B901 Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.

These aren't taken into account with how the linter is currently set up, for that i need to manually select= all of the current linting categories via setup.cfg, which I might want to do in another PR.

[ShadowJonathan]

(also before anyone comments on it, no, i cant make this PR smaller (than 200 lines), because i have to both add flake8-bugbear and fix all the small problems that pop up while going over the new linting suite errors)

[clokep]

@ShadowJonathan When doing large repetitive changes like this it can help to do one of the following:

• Do not enable all of the checks at once and do them in separate PRs.
• Make the fixes in separate commits which clearly state what each commit is fixing ("Do not use mutable default arguments.", etc.)

Also note that a PR above a couple of hundred lines of diff generally takes much longer to review. Small PRs are better!

[ShadowJonathan]

I could do this if there was a select= key for flake8 in setup.cfg, which makes me able to enable sub-categories for each of the checks and then create PRs for each, but right now i'd rather want to present this one (which will take a second, which is fine for me) instead of making the select= PR first and then waiting for that one before making and chopping up this one.

(The problem is is that i cant "enable just some of the checks", flake8 works either based on defaults with ignored checks, or selected checks (together with ignored ones).

I could make PRs that in parallel that have every other category explicitly ignored other than the one that the PR is for, but that'd create a mess (you'd have to merge ignore=B001,B002 with ignore=B002,B003, and then do this again with ignore=B001,B003. I could rebase every PR when previous ones are merged, but that'd be tedious and confusing, y'all have already indicated you don't want rebased PRs without reason.

Finally I could also then just bring those "small fix PRs" in serial, but that'd take a while to be fixed, i'd have to keep track of every separated change locally (which may slowly become out of date), and a bunch of other stuff that's just plain overhead.

If the select= was present, then i could just do select=...,B001, select=...,B002, select=...,B003 PRs, which then are able to be merged together more nicely (select=...,B001,B002), and when done, a final PR could make it just the broad B category, but alas.)

TL;DR: Next time I'll make sure the small changes are better able to be applied into seperate PRs. 👍

[clokep]

Should we update this to be str(e)?

[ShadowJonathan]

DependencyException.message is actually a property that formats it properly, thus the noqa, this is in a few other places too

[clokep]

Overall I like the direction this is heading; would you mind restructuring it so it's easier to review?

I think that many of the changes in here are good (I'm especially happy to see the change to stop using mutable input arguments), but this PR tries to do too many unrelated things and it hinders discussion of individual changes. I'm going to close this so that we can start afresh.

In particular I'm afraid of introducing regressions and it being difficult to bisect exactly what caused it. I would suggest splitting this work up into smaller pieces. A few ways this could be done:

Strategy 1

• Split up the PR into separate types of changes (e.g. one for the mutable input arguments, one for using keys() instead of items(), etc.), this could be done by running flake8-bugbear locally and fixing the issues.
• After those have been merged, introduce flake8-bugbear and enable it. This might involve fixing some minor discrepancies that occurred in the meantime.

Depending on the size of each of the changes, some of them could be grouped together if each change is in separate commit with a clear commit message.

Strategy 2

• Introduce flake8-bugbear and disable most of the checks.
• Fix the issues found and create a new PR (essentially a scaled down version of this).
• Create subsequent PRs (in series or in parallel) that fix remaining issues.

This will cause conflicts, but they will be easy to fix since it is just the single line of which checks are enabled. As with above, some of the changes could be grouped if they're done in separate commits.

[ShadowJonathan]

I'm going to take a variant of strategy 2; i'll create a PR (like mentioned before) that'll explicitly enable checks in setup.cfg (on separate lines), once that's merged, i'll create a few PRs that all individually enable checks on bugbear and then also subsequently resolve their issues, i'll lead this with a PR that'll add flake8-bugbear to the dependencies, and enable the checks that do not cause issues. I'll trail the induvidual PRs with a final PR that'll condensate the B001,B002,B003 into just B and the like.

Does that sound good?

TL;DR: PR that makes select= explicit -> PR that adds flake8-bugbear and checks that dont cause issues -> *(PRs that add check and subsequent fixes) -> trailing PR that'll replace individual close-scoped B001 statements with just B

[clokep]

I think that sounds reasonable! Thanks for being understanding!

[ShadowJonathan]

(note to self; make tracker issue for it)