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
Adds literal node #865
base: master
Are you sure you want to change the base?
Adds literal node #865
Changes from 6 commits
3d3ae1d
9338386
b78d4dc
6d302ab
3c115d7
1592df4
3b8a82f
80f9861
0737ec5
eb4b20c
a0e2b35
f3b3f75
694a032
1bd9c93
fb32fa5
89414a8
c86cec5
59eb792
5d1b801
e2d950f
4fa9593
d321a4a
b69fd38
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.
Hmm, why is this needed? Isn't
Literal
always fromtyping_extensions
in thiselse
branch?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 what's happening here is that:
python 3.8 and later has
typing.Literal
which has an__origin__
attributepython 3.7
typing_extensions.Literal
has an__origin__
attributepython 3.6 and earlier
typing_extensions.Literal
does not have an__origin__
attribute or any equivalent attributeAND
so as far as I can tell, for 3.6 and earlier the only way to check for Literalness is using the string. This is gross but I don't know of a better way.
This logic is unnecessarily complicated though. How do you feel about
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.
There is an
is_literal_type()
function here which also work with Python 3.6. The solution to the problem seems very stupid, you have to import a hidden class_Literal
:I have no idea why.
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.
Before 3.6 typing_extensions creates the internal
_Literal
class using atyping._FinalTypingBase
mixin https://github.com/python/typing/blob/master/typing_extensions/src/typing_extensions.py#L344 , and then Literal is an instantiation of that class. So type(a) returns the outer _Literal class instead of the instantiation or something. Anyway this seems less bad than the string hack, so I'll go with 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.
Maybe make these branches explicitly correspond to python versions. So I think
but I'm not really sure about 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.
Why do you feel this is better? Philosophically I tend to think that directly checking is the right way to do things, in case e.g. the attribute name every switches back or something equally cursed. This also has the benefit that mypy/pylance/other syntax parsers can correctly infer that the attribute exists, which is left implicit if you're going by version.
If this is about readability, I'd be happy to add comments saying why the check is necessary.
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.
Right, I was just thinking that at some point in the future, omegaconf might want to drop support for Python 3.6 and then it would be useful that you can directly see that this if-else check has become unnecessary.
But, yes, a comment is likely already sufficient.