-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Resolve new flake8 error #4462
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
Resolve new flake8 error #4462
Conversation
It's a flake8-bugbear bump, I expect. You'll need to clear your pre-commit cache locally to see it, as we don't pin bugbear. |
Is there a reason to remove the comment? That seems helpful to both explain why there's an expected exception, as well as if it ever does change, we'd know to just add the new failure there too. |
cf184f3
to
f87da1b
Compare
I wasn't sure, tweak or remove. After seeing your feedback I tweaked now instead of removing. What do you think? |
Thanks for the hint! Confirmed, after
Could we pin bugbear? Getting surprised in the middle of something else can be quite distracting. |
@rwgk Upgrading just becomes a bit annoying since we don't have any autoupdate functionality for pinned additional_dependencies. |
Thanks! In that case I think not pinning is fine. Next time I'll be less surprised :-) Does the changed comment look good to you? |
Yeah, looks good. |
Thanks! |
* Resolve flake8 error by replacing `pytest.raises(Exception)` with `SystemError` * Also remove the obsolete comment. * Tweak comment instead of removing it.
Description
Resolve this new failure:
PR #4461: https://results.pre-commit.ci/run/github/38581626/1674006122.PZIdTNh0SNa60OuNgaDU9w
For simplicity & safety, giving up on the idea to future-proof the test (see removed comment).
The danger pointed out by flake8
B017
is real, and currently all Python versions generate aSystemError
.Minor remarks for completeness:
pre-commit clean
Suggested changelog entry: