fix string normalization #33
Conversation
Codecov Report
@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 78.34% 78.52% +0.17%
==========================================
Files 4 4
Lines 956 964 +8
Branches 219 223 +4
==========================================
+ Hits 749 757 +8
Misses 167 167
Partials 40 40
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This change updates annotation comparison to first normalize the original string leaf nodes to their repr form which is what is generated from the ast (the pyi file).
terencehonles
force-pushed
the
fix-string-normalization
branch
from
8041d83
to
1731abf
Compare
|
|
||
| node.children = [normalize_node(i) for i in node.children] | ||
| if isinstance(node, Node): |
There was a problem hiding this comment.
I probably should have done this in #30, but since it won't touch the children it doesn't have to touch
|
|
||
| return node | ||
|
|
||
|
|
||
| def maybe_replace_any_if_equal(name, expected, actual, flags): |
There was a problem hiding this comment.
I'm a little unsure of this function... if the strings are equal shouldn't the original be used rather than the one we generated? I believe the return should probably be actual not expected (and I'm not sure why a fallback is given, but it could be given in reverse order).
I'm also a little confused about the Any normalization. From the comment it seems like people should be able to keep their original Any, but if you don't use --replace-any then it will error out instead of leaving things as is, but if you use --replace-any it doesn't check for nested Anys and was very naively stripping quotes. I'm guessing it probably was OK, but I updated this to look at the node type in order to strip the quotes and used the normalized string so that it will always be a single single quote (I'm not sure if """ or ''' are allowed for Leaf(STRING) values).
There was a problem hiding this comment.
Hmm, I can't decide on this. This part was mostly written by Lukasz so don't have a good answer here.
There was a problem hiding this comment.
I'm not in a rush on this, and I'm thinking we should probably be recursively checking for Any in case it's wrapped in something like Optional[Any] or Dict[str, Any] etc, but I'm not sure if --replace-any is targeting strictly Any that is not contained in anything else.
This code also seems like it's allowing a string form of Any to be normalized into a non string form of Any so checking nodes directly would not be sufficient (and why it's str(node) first) since Leaf(STRING, "'Any'") != Leaf(NAME, "Any") (and other Any forms that are captured) so if it was recursively allowing Any it would not be as trivial as looking for Leaf(String) and normalizing Anys to a single form "like" what I'm doing to normalize to the repr form.
| @@ -53,30 +53,33 @@ def test_can_run_against_current_directory(tmp_path): | |||
| class RetypeTestCase(TestCase): | |||
| maxDiff = None | |||
|
|
|||
| def assertReapply( | |||
There was a problem hiding this comment.
I accidentally had too much whitespace in my expected output, but I was stepping through what the assertion was doing to figure out if I should be using a different one and I realized they were all duplicates some of which having slightly different orders so I figured I'd remove that duplication.
This change updates annotation comparison to first normalize the original string leaf nodes to their repr form which is what is generated from the ast (the pyi file).
fixes #24