Skip to content
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 ruff's flake8-bugbear ruleset #2341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enable ruff's flake8-bugbear ruleset #2341

wants to merge 1 commit into from

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Dec 20, 2024

Status

Ready for review

Description

bugbear was one of the most popular flake8 plugins and now ruff supports it. Running it found a number of missing "assert" statements, among other issues I all found valuable to fix:

  • Useless statements that were probably missing a leading "assert"
  • Mutable default arguments that should be constructed at function runtime
  • Unused loop variables that don't start with _
  • Usage of str.strip() with a multi-character string

I suppressed three types of errors:

  • Raising an exception inside an exception handler without using from err; would be nice to address later, but there were too many to assess.
  • Loop variables being bound past their lifetime; the two uses were only in tests and safe so I applied individual suppressions as we'd want to flag problematic uses in real code.
  • pytest.raises(Exception) is evil, but also not a priority to fix in tests.

Test Plan

  • visual review
  • CI passes

@legoktm legoktm requested a review from a team as a code owner December 20, 2024 01:05
bugbear was one of the most popular flake8 plugins and now ruff supports
it. Running it found a number of missing "assert" statements, among
other issues I all found valuable to fix:

* Useless statements that were probably missing a leading "assert"
* Mutable default arguments that should be constructed at function
  runtime
* Unused loop variables that don't start with _
* Usage of str.strip() with a multi-character string

I suppressed three types of errors:
* Raising an exception inside an exception handler without using
  `from err`; would be nice to address later, but there were too many
  to assess.
* Loop variables being bound past their lifetime; the two uses were only
  in tests and safe so I applied individual suppressions as we'd want to
  flag problematic uses in real code.
* `pytest.raises(Exception)` is evil, but also not a priority to fix
  in tests.
@legoktm
Copy link
Member Author

legoktm commented Dec 20, 2024

The missing asserts have revealed that some tests were silently failing and will need some fixes...

@cfm cfm assigned cfm and unassigned cfm Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants