-
-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable flake8-bugbear rule #2807
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2807 +/- ##
==========================================
+ Coverage 99.13% 99.15% +0.02%
==========================================
Files 115 115
Lines 17242 17261 +19
Branches 3085 3086 +1
==========================================
+ Hits 17093 17116 +23
+ Misses 104 101 -3
+ Partials 45 44 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big approve on enabling bugbear, bunch of small style comments.
"RUF", # Ruff-specific rules | ||
"E", # Error | ||
"F", # pyflakes | ||
"E", # Error | ||
"W", # Warning | ||
"I", # isort | ||
"B", # flake8-bugbear | ||
"YTT", # flake8-2020 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you moved "E"
... probably want this list sorted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider running a toml-formatter in pre-commit that does it automatically
( | ||
SSL.OP_NO_QUERY_MTU | ||
| SSL.OP_NO_RENEGOTIATION # type: ignore[attr-defined] | ||
) | ||
SSL.OP_NO_QUERY_MTU | SSL.OP_NO_RENEGOTIATION # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this formatting change due to a version bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's changed by black funnily enough, not ruff.
Tested with reverting changes and running pre-commit locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's a good change - but I'm confused why it's in this PR and not in a separate PR that does the actual version bump in the committed config file.
All but the one in `trio/_tests/test_socket.py` line 390 because it already has a lot of comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit shitty to put the burden of cleaning up old smelly code in an entirely separate PR that just happens to touch those lines, so feel free to ignore ones you don't want to bother with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great~
This PR enables the flake8-bugbear rule and fixes all the existing issues that were raised.