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

B022: No arguments passed to contextlib.suppress #231

Merged
merged 6 commits into from
Mar 20, 2022

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Mar 17, 2022

Closes #222.

This covers the case where contextlib.suppress is used

import contextlib

with contextlib.suppress():  
    raise ValueError

However, it does not currently cover the case where suppress is imported from contextlib

from contextlib import suppress

with suppress():  
    raise ValueError

Do you know of any existing flake8 helper functions/methods to tie the suppress function node to it's import?
I don't want to just ignore the one word suppress without checking incase it's actually a user defined function.

@cooperlees
Copy link
Collaborator

You're to quick - Sorry - I just merged another PR. I can look at cleaning up here or if you beat me that would be awesome too :D

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 20, 2022

You're to quick - Sorry - I just merged another PR. I can look at cleaning up here or if you beat me that would be awesome too :D

hehe no worries, I've resolved them now 😄

Would be good to get your take on this PR currently only covering contextlib.suppress rather just suppress since we'd have to resolve scope to figure out that the suppress being called is definitely the one from contextlib?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All makes sense to me, and test show us working and handling * expansion which is good.

@cooperlees
Copy link
Collaborator

Would be good to get your take on this PR currently only covering contextlib.suppress rather just suppress since we'd have to resolve scope to figure out that the suppress being called is definitely the one from contextlib?

Ahhh, I didn't think of this. Yeah, I feel assuming with suppress would be to noisy and we shouldn't do that.

README.rst Show resolved Hide resolved
@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 20, 2022

Ahhh, I didn't think of this. Yeah, I feel assuming with suppress would be to noisy and we shouldn't do that.

Yeah that was my thinking too, better to avoid false positives 😄

Maybe we initially keep the rule like this and then I can make a separate issue to find a way to track variable/function scope when traversing the AST as a future refinement (I think pyflakes has something similar for tracking if imports get used).

@cooperlees cooperlees merged commit e389294 into PyCQA:master Mar 20, 2022
@jpy-git jpy-git deleted the b022_contextlib_suppress branch March 20, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect when no arguments were passed to 'contextlib.suppress'
2 participants