TRIO112: replace one-line nursery with regular function call #34
Conversation
jakkdl
changed the title
jakkdl
force-pushed
the
trio112_single_statement_nursery
branch
2 times, most recently
from
0270629 to
28e282e
Compare
tests/trio112.py
Outdated
| # safe? | ||
| with trio.open_nursery() as n, open("") as o: | ||
| n.start(...) |
There was a problem hiding this comment.
Bit suspicious for #22-like reasons, but since violating this rule is more of a style problem than a correctness issue let's err on the side of less alarms.
There was a problem hiding this comment.
Thinking about this one more, is it actually that big of a style problem to have multiple withitems in a single with? And if the open was placed in it's own with it'd clearly be a TRIO112 (or TRIO302/111).
I changed it to be an error, but trivial to change it back.
jakkdl
force-pushed
the
trio112_single_statement_nursery
branch
from
28e282e to
c57790b
Compare
|
rebased onto main, added support for multiple withitems, cleaned up code. |
| @@ -333,10 +353,30 @@ def has_exception(node: Optional[ast.expr]) -> str: | |||
|
|
|||
|
|
|||
| class Visitor102(Flake8TrioVisitor): | |||
| class TrioScope: | |||
There was a problem hiding this comment.
get_matching_call (previously get_trio_scope) no longer constructs a TrioScope, since only Visitor102 actually cared about it. So moved it to a subclass
There was a problem hiding this comment.
IMO this isn't an affirmative reason to move it, so I'd keep the diff a bit smaller by leaving it where it was. On the other hand, nor is there any real reason to move it back!
There was a problem hiding this comment.
haha. I liked subclassing it for people reading it later on not encountering it through a commit diff, fun tension
| # isinstance(..., ast.Call) is done in get_matching_call | ||
| body_call = cast(ast.Call, node.body[0].value) |
There was a problem hiding this comment.
I'd use an assert rather than a cast() here.
There was a problem hiding this comment.
assert is actually not possible here, the isinstance call is done later inside get_matching_call (which if it inside there evaluates false propagates to the condition calling it being false).
the easiest alternative is a redundant isinstance in the condition
I suppose the cast is incorrect in that body_call isn't actually a Call yet, but is for brevity/convenience later.
| @@ -333,10 +353,30 @@ def has_exception(node: Optional[ast.expr]) -> str: | |||
|
|
|||
|
|
|||
| class Visitor102(Flake8TrioVisitor): | |||
| class TrioScope: | |||
There was a problem hiding this comment.
IMO this isn't an affirmative reason to move it, so I'd keep the diff a bit smaller by leaving it where it was. On the other hand, nor is there any real reason to move it back!
Final remaining issue without a PR 💪
Code is fugly, will make a pass tomorrow, but I think the check is done.