Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Explicitly add flake8 selections #9370

Closed

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Feb 10, 2021

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)

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

Part of #9366

@ShadowJonathan
Copy link
Contributor Author

I used flake8 setup.py -vvvv 2> flake8.txt and https://flake8.pycqa.org/en/latest/user/violations.html#selecting-violations-with-flake8 to determine which selections are default, apart from flake8's default, C4 (flake8-comprehensions) was the only other check that was turned on by default.

@clokep
Copy link
Member

clokep commented Feb 10, 2021

I'm a bit confused by this -- I thought the goal was to enable additional checks only for flake8-bugbear? If that's not possible, then I think we'll need to ignore all the ones that don't pass yet.

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Feb 10, 2021

I outlined it here, but TL;DR: It's easier to individually add the checks to select= rather than remove them from ignore= one-by-one, the latter is more susceptible to merge conflicts, and it's easier to make git figure out the diff being "ah, just adding some lines here" rather than "oh, i need to remove this line, and this line, and this line...".

It's also simply easier to work with, and easier to observe explicitly enabled > implicitly enabled, i already created some other issues which touched on those explicitly-disabled checks, to create discussion to be able to remove them.

Edit: Just read your comment again, yes, that's the goal, but this is the first PR required for that, i isolated the changes to be small to this, and the next PR will be adding flake8-bugbear and non-complaining checks, once that one's merged, i'll add the one-by-one checks and the enabling in the config. See #9366.

@anoadragon453 anoadragon453 requested a review from a team February 22, 2021 12:10
@clokep
Copy link
Member

clokep commented Feb 24, 2021

I'm not too keen to merge this as I'm not a huge fan of disabling these checks (i.e. if we never finish a project to enable bugbear and new checks get added to flake8 then we'll be missing out). I think the approach should be to disable just the bugbear checks and start fixing those issues individually.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants