Enable flake8-bugbear rules via ruff #7403
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Switch to f-string and remove no-longer needed override.
Address most of the issues found by the flake8-bugbear rules, and suppress a few that aren't worth fixing right now. The remaining ones will be fixed in follow-up commits.
As described in <https://docs.astral.sh/ruff/rules/jump-statement-in-finally/> and more recently in <https://peps.python.org/pep-0765/>, using return in a finally block has the potential of suppressing exceptions. Given that in both cases we have a blanket exception handler, it should be safe to return outside of a finally. If there are exceptions being suppressed, we should surface them.
As described in <https://docs.astral.sh/ruff/rules/assignment-to-os-environ/>, assigning to os.environ doesn't actually clear the environment (i.e. remove keys that are not set in the new value). I think it is better to just explicitly suppress the warning since the existing code works and isn't worth modifying.
flake8-bugbear noticed that the second loop was checking the wrong value. I noticed that the test wouldn't actually fail if the expected exception wasn't raised, so use pytest.raises, which will correctly fail if it doesn't work as expected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review
Description of Changes
Upgrade ruff to 0.8.6
Switch to f-string and remove no-longer needed override.
Enable flake8-bugbear rules via ruff
Address most of the issues found by the flake8-bugbear rules,
and suppress a few that aren't worth fixing right now. The remaining
ones will be fixed in follow-up commits.
Don't use finally to return a redirect
As described in
https://docs.astral.sh/ruff/rules/jump-statement-in-finally/ and more
recently in https://peps.python.org/pep-0765/, using return in a
finally block has the potential of suppressing exceptions.
Given that in both cases we have a blanket exception handler, it should
be safe to return outside of a finally. If there are exceptions being
suppressed, we should surface them.
pretty_bad_protocol: Suppress warning about modifying os.environ
As described in
https://docs.astral.sh/ruff/rules/assignment-to-os-environ/, assigning
to os.environ doesn't actually clear the environment (i.e. remove keys
that are not set in the new value).
I think it is better to just explicitly suppress the warning since the
existing code works and isn't worth modifying.
Have migration test actually fail if value isn't missing
flake8-bugbear noticed that the second loop was checking the wrong
value. I noticed that the test wouldn't actually fail if the expected
exception wasn't raised, so use pytest.raises, which will correctly fail
if it doesn't work as expected.
Testing
How should the reviewer test this PR?
Deployment
Any special considerations for deployment? n/a
Checklist
make lint) and tests (make test) pass in the development containermake -C admin test) pass in the admin development container