reimplement trio100 & trio101 with libcst #142
Conversation
|
Ah, looks like the CI failure is due to shed not fixing everything in one go. I've noticed that happening frequently which is somewhat annoying, will track down the cause and open an issue in shed. |
jakkdl
force-pushed
the
libcst_rewrite
branch
from
c77b2a8 to
3c85498
Compare
|
Fixed the CI, and cribbed your try/finally solution for |
flake8_trio/visitors/helpers.py
Outdated
|
|
||
| return m.matches( | ||
| func, | ||
| m.FunctionDef( |
There was a problem hiding this comment.
I'm generally a little wary about such deeply-indented nested calls; maybe the m.Decorator(...) could be pulled out as a named variable?
There was a problem hiding this comment.
pulled out a list_contains - it's close to being usable in with_has_item as well, will likely iterate on these as I use them in more rewritten visitors/transformers.
| return updated_node | ||
|
|
||
| @m.visit(m.Await() | m.For(asynchronous=m.Asynchronous())) | ||
| # can't use m.call_if_inside(m.With), since it matches parents *or* the node itself |
There was a problem hiding this comment.
We might want to write a helper function for this at some point?
There was a problem hiding this comment.
Oooh, I hadn't even considered writing my own decorators. Hell yes I want this, would be very useful in shed as well - I'm pretty sure there's at least one bad matcher due to this. But yeah, for another PR.
jakkdl
force-pushed
the
libcst_rewrite
branch
from
3c85498 to
cf468bf
Compare
you keep underestimating how good the [infra]structure/patterns I've built up are 😁 Though I did spend a lot of time on polishing this, and there's quite a bit of potential future gnarl left. |
Hofstadter's law has earned my respect, but you're right - kudos! |
jakkdl
force-pushed
the
libcst_rewrite
branch
from
cf468bf to
3feea1a
Compare
|
Tweaked and ready to be shipped! And good call, this is nicer. |
| # need to use Union due to https://github.com/Instagram/LibCST/issues/870 | ||
| def checkpoint_node(self, node: Union[cst.Await, cst.For, cst.With]): | ||
| if self.has_checkpoint_stack: | ||
| self.has_checkpoint_stack[-1] = True |
There was a problem hiding this comment.
Turns out getting future annotations with get_type_hints is not possible, though I think an alternative to using Union is stringifying the annotation.
https://stackoverflow.com/questions/66006087/how-to-use-typing-get-type-hints-with-pep585-in-python3-8
There was a problem hiding this comment.
See https://discuss.python.org/t/pep-649-deferred-evaluation-of-annotations-tentatively-accepted/21331 for background, this is a long-running problem which should at least get better from 3.12 😌
There was a problem hiding this comment.
Time to re-read that PEP again :D
Oh and this will solve @dataclass annotation troubles as well!
Also changed how trio100 works to avoid missed cases where a function is defined inside the with statement.
100 & 101 moved into their own files.
I spent some time trying to get batching to work,
BatchedVisitoris not compatible withMatcherDecoratableVisitor- and especially not with transformers. Tried dynamically creating a class that inherited from all enabled visitors, but ran into so many minor headaches with that approach I abandoned it. Might revisit once all visitors are changed to cst, but for now it's certainly not worth the effort for the sake of speeding it up. There might be a case of wanting to do the transforms simultaneously in case one transform unlocks another, and stuff like that.I thought batching would be required, since the utility visitors are separate classes, but I think that can be made to work by having error classes that require the functionality of a utility visitor to inherit from it. But neither 100 nor 101 require any of the utility ones so didn't bother with it too much.
I'm waiting with enabling transforming for trio100 for a later PR, since that will require a bunch of UI changes / flags / etc.
Ended up opening a bunch of issues in pyright/libcst/mypy during the process, Instagram/LibCST#870 is probably the most relevant one - which is probably gonna be somewhat of a pain going forward if not fixed. (I didn't bother trying to stop shed from removing
Union, but if possible that'd make it less painful.)