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
Native python types #8559
Native python types #8559
Changes from 4 commits
2a8177c
708d8f2
a5a4f6e
eeb132d
61f0984
6d6efc1
2daf160
8bd58d3
cc4d87a
bfd4e55
94ac77f
6d61bbd
6f71a84
d430ed2
2aa2eb9
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.
Do we want to support this, given that the number type has been deprecated?
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.
Deprecated, but not gone.
I have no strong opinion on this
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.
After giving it some more thought, me neither. At some level I'd prefer not to integrate deprecated types into new features, but if it's as simple as this I guess it doesn't hurt too much either. The only thing is perhaps that we have the opportunity to make a clean cut here, to be more visible than the current deprecation warning.
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.
So I think we have to be more careful here.
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.
Why? That seems entirely as expected?
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.
But you call it on
tt
(notpython_type
), and filter them out if they are optional. So suppose you'd havepython_type=Union[int | Optional[str]]
(admittedly a convoluted expression but still), wouldn't this evolve toUnion[int, None]
, dropping thestr
because it is "optional"?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.
When you write it all without quotes, Python probably collapses them together so you can't get in this scenario. But then we have to be very careful about this case when we add support for quoted 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.
Or even
python_type=typing.Union[int, typing.Annotated[typing.Union[str, None], "myannotation"]]
. Python can not collapse it there.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.
Then again,
typing_inspect.is_optional_type
doesn't seem to work at all ontyping.Annotated
.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
Union[int | Optional[str]] == Union[int | str | None]
Optionals always flatten
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.
Plain ones do, but not if something non-trivial pops in there. If we plan to never support strings, that's one thing covered. That only leaves
Annotated
then, but that might not be a concern right 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.
I think this one should also be easy enough to support for 7.4