-
Notifications
You must be signed in to change notification settings - Fork 191
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
Support PEP 604 in ApplyTypeAnnotationsVisitor #868
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
==========================================
+ Coverage 94.79% 94.93% +0.13%
==========================================
Files 249 254 +5
Lines 25831 25923 +92
==========================================
+ Hits 24487 24609 +122
+ Misses 1344 1314 -30
... and 42 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
The CI failures on Python 3.11 are unrelated: |
retype is no longer maintained. Since it's based on lib2to3, its corpse is not going to be useful for too long. merge_pyi now just wraps libCST, so mention that. If someone seeing this is looking for things to do, I think libCST's support here could be improved. E.g., I filed Instagram/LibCST#868 But maybe that should convert to typing.Union. I'm sure there are other things that could be improved!
retype is no longer maintained. Since it's based on lib2to3, its corpse is not going to be useful for too long. merge_pyi now just wraps libCST, so mention that. If someone seeing this is looking for things to do, I think libCST's support here could be improved. E.g., I filed Instagram/LibCST#868 But maybe that should convert to typing.Union. I'm sure there are other things that could be improved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this took a while to get to. LGTM but let me ping @stroxler in case he wants to take a look too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than I think it may be worth adding one more test.
Thanks for this!
after=after, | ||
overwrite_existing_annotations=True, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding a test showing the import collector works (that's where a lot of the new logic lives, and I don't think we have anything exercising that code yet)
@hauntsaninja do you think you'll have time to add an extra test? |
Thanks for the suggestion! I added a test, then noticed that the collectors don't reach all the way into e.g. Instead of effectively writing my own visitor to get all the nodes, I decided to just use the LibCST visiting. So in I think the net result is overall a little simpler, but ended up being a larger change than I was expecting to make. |
cc @carljm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks!
No description provided.