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
Fix
Union[..., NoneType]
injection byget_type_hints
if aNone
default value is used. #482base: main
Are you sure you want to change the base?
Fix
Union[..., NoneType]
injection byget_type_hints
if aNone
default value is used. #482Changes from 13 commits
376ae56
24c5602
7f000b1
8cc4464
881ffee
58082b9
5c94bd6
b42e203
ed552e4
313ddd8
3ba4ee7
6ffcd23
403e7ce
52c93e9
a1777f3
9e59796
1761a43
54b8eb0
5612f6f
91075ff
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.
Concatenating these seems a bit odd. In any case,
type_hints
is a dictionary, so the str() and repr() are the same.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.
Thanks for pointing this out I was not 100% sure here and kept a lazy version. Changed to only
repr
.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 shouldn't this happen? You can put
None
in annotations.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.
At this point a None value should already be converted to
NoneType
inoriginal_hints
. I think these two lines are redundant but to be safe I added the double check.Added a comment to clarify 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.
Would this also work?
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.
Yes it would. However there are minor differences involving identities and caching.
If a ForwardRef is involved
original_evaluated
can be a new object as_tp_cache
is not queried. Not changinghints["x"]
will return an object that matchesexpected["x"]
by identity in more cases.That's a minor feature and I am fine with dropping it if you want; for now added a comment to clarify this.