B904: ensure the raise is in the same context with the except #191
Conversation
|
@Zac-HD - Thoughts? @isidentical - Sorry - we run black formatting with an experimental feature:
|
|
@cooperlees ah, my bad. By the way, is there a particular reason why this project doesn't use |
|
The concept sounds good to me, though the diff looks like it's patching B902 instead of B904? (this also seems pretty rare to me, but as a general rule "if someone provides a patch, it matters enough to merge") |
|
No actually not, I do not need to change anything on the B904's implementation, since it just checks if there are any except handlers within the current |
isidentical
force-pushed
the
b904-context
branch
from
ac1a65e
to
f066f82
Compare
There is no real reason other than I dislike pre-commit - But I deal with it since it seems the norm in the open source world. My editor keeps me black formatted and running the tests here are easy so I just do it manually. I would accept a pre-commit or something to make it easier for contributors tho. |
| @@ -490,7 +524,7 @@ def check_for_b902(self, node): | |||
| # `cls`? | |||
| return | |||
|
|
|||
| bases = {b.id for b in self.node_stack[-2].bases if isinstance(b, ast.Name)} | |||
| bases = {b.id for b in cls.bases if isinstance(b, ast.Name)} | |||
There was a problem hiding this comment.
Cleaner, but it's not really used anywhere else, but I do agree this makes it easier to know what it is, if you know what cls generally means ...
There was a problem hiding this comment.
I think cls makes a bit more sense than something[-2] (which is a lot less clear).
|
Very occasionally, a more nested pattern comes up, such as trying a backport import as well. I don't think I actually have any code that does this, but would this change also avoid the lint message in this case? try:
from preferred_library import Thing
except ImportError:
try:
from fallback_library import Thing
except ImportError:
class Thing:
def __getattr__(self, name):
raise AttributeError |
It should work for that case as well, will add it to the tests. |
|
Thanks @cooperlees! I'll try to address your review.
I assume this was not targeted at me, but I'm kind of interested in contributing more. Would love to help with other duties as well (PR reviews etc.). |
Co-authored-by: Cooper Lees <me@cooperlees.com>
|
This is only failing on black. I'll fix before merging. If you get time, please reformat. Otherwise I'll take care of it. |
|
@isidentical - Sorry it's taken me so long but now this with a rebase to master is unhappy. Do you have any cycles to clean this up? Was looking to release today. I'll see if I can understand too now or I might release without this and we just cut another release when you get time. |
bugbear.py
Outdated
| @@ -168,6 +181,7 @@ class BugBearVisitor(ast.NodeVisitor): | |||
| node_window = attr.ib(default=attr.Factory(list)) | |||
| errors = attr.ib(default=attr.Factory(list)) | |||
| futures = attr.ib(default=attr.Factory(set)) | |||
| context = attr.ib(default=attr.Factory(list)) | |||
There was a problem hiding this comment.
Is this all that is needed? How did this ever work if so :|
| context = attr.ib(default=attr.Factory(list)) | |
| contexts = attr.ib(default=attr.Factory(list)) |
Change the self.context to self.contexts ...
There was a problem hiding this comment.
Ok - That makes sense to me. Will give @isidentical a while to respond. No idea how that self.contexts got changed to self.context ... Unless it was a refactor / push error.
Resolves #190. Introduces a simple version of context management, though more detailed analysis might need something a little bit specialized in the future (e.g stuff like this).