Full handling for applying type comments to Assign #599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: In the previous PR, I added basic support for converting an Assign with a type comment to an AnnAssign, as long as there was only one target. This PR handles all fully PEP 484 compliant cases: - multiple assignments - multiple elements in the LHS l-value We cannot handle arity errors because there's no way to do it. And we don't try to handle the ambiguous case of multiple assignments with mismatched arities (PEP 484 isn't super clear on which LHS is supposed to pick up the type, we are conservative here). The ambiguous case is probably very uncommon in real code anyway, multiple assignment is not a widely used feature. NOTE: my local pyre is complaining and I don't understand it, putting a PR up anyway to see what CI does. Test Plan: There are new test cases covering: - multiple elements in the LHS - multiple assignment - both of the above together - semicolon expansion, which is handled differently in the cases where we have to add type declarations - new error cases: - mismatched arity in both directions on one assignment - mismatched arity in multiple assignment ``` > python -m unittest libcst.codemod.commands.tests.test_convert_type_comments ..... ---------------------------------------------------------------------- Ran 5 tests in 0.150s OK ```
facebook-github-bot
added
the
CLA Signed
Codecov Report
@@ Coverage Diff @@
## main #599 +/- ##
==========================================
+ Coverage 94.72% 94.78% +0.05%
==========================================
Files 244 245 +1
Lines 24916 25101 +185
==========================================
+ Hits 23602 23791 +189
+ Misses 1314 1310 -4
Continue to review full report at Codecov.
|
|
Looks like I'll need to bump the required python version or find a 3.8 substitute for ast.unparse |
stroxler
force-pushed
the
convert-type-comments--pep-526-flattening
branch
from
3e96c90 to
47edbbc
Compare
stroxler
commented
Jan 13, 2022
stroxler
changed the title
There were three different layers of target things, which meant using target / targets as a naming convention was not working well. Now, I say: - a target is one or more `target` values - a target is a single lvalue, and maps to a `bindings` object of one or more bindings - a binding is a specific, single value that can be annotated Similarly, there are two layers in annotation handling: - a type comment corresponds to an annotations, one or more types - an annotations atom is an annotation When zipping, we zip a single bindings against a single annotations. When handling multi-assignment Assigns, we have to do this multiple times and concatenate the results.
stroxler
changed the title
This was referenced Jan 28, 2022
1 task
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In the previous PR, I added basic support for converting an
Assign with a type comment to an AnnAssign, as long as there was
only one target.
This PR handles all fully PEP 484 compliant cases:
We cannot handle arity errors because there's no way to do it. And
we don't try to handle the ambiguous case of multiple assignments with
mismatched arities (PEP 484 isn't super clear on which LHS is supposed
to pick up the type, we are conservative here). The ambiguous case is
probably very uncommon in real code anyway, multiple assignment is not
a widely used feature.
NOTE: my local pyre is complaining and I don't understand it, putting
a PR up anyway to see what CI does.
Test Plan
There are new test cases covering:
where we have to add type declarations