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.
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
reimplement 91X in libcst #143
reimplement 91X in libcst #143
Changes from all commits
df51441
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think here you mean
bound=Union[...]
? The multi-arg form means "must be an exact match to one of these types".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.
Confused me for a minute why the multi-arg form doesn't give errors when passed subtypes, but figured out that it transforms the type.
And mypy gave me errors on the union one, but that's a bug with a PR in the pipe: python/mypy#14755
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.
major hoops to have to run through, but didn't find any better way of doing it for now.
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.
Seems surprising to return
None
on failure, instead of raising an exception - you lose the ability to distinguishNone
fromfoobar()
(which raises ValueError as it's not a literal).We might also want to open an upstream issue - there's
Integer.evaluated_value
for example, but no such property on e.g. lists or tuples and fixing that would let future projects avoid this helper function.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.
Oh, in the case ofiter_guaranteed_once
(literal parameter to range) it's actually plenty fine to use Integer/Float.evaluated_value;edit: nvm I forgot about range-based for loopsedit 2: nvm what actually matters isedit 3:range(5+5)
orrange(10, 5, -1)
ast.literal_eval
doesn't handle5+5
anyway 😂I opened Instagram/LibCST#878
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.
ugly, but works pretty much like the ast version.
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.
I'd be inclined to do the same "unparse and use ast" trick - if not we'll need some serious testing to be sure they're equivalent.
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.
you mean using
iter_guaranteed_once
?It has all the same tests that the ast version had, and vast majority of the logic is the same, so I'm pretty confident it's equivalent actually. And if/when 103/104 is converted to libcst there will be no ast users left using it.