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 3 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.
I'm actually not sure about hardcoding this mapping to the type objects. I think I'd prefer to use the existing
inmanta.ast.type.TYPES
mapping, so that if we ever change something there it is reflected in both flows. Then this mapping here would simply be a mapping to the model types as represented in the model, e.g.str -> "string"
.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.
Upon closer consideration, what I had here was even wrong.
inmanta.ast.type.TYPES
is the internal attribute types, which are subtly different from what is here, mapping it twice won't be correct / consistent with what existed already.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'm not following with this. From your comment I understood that you agreed we should use
inmanta.ast.type.TYPES
, but from the implementation I think I misunderstood? Why should we not use it?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 don't think we should use
inmanta.ast.type.TYPES
because:inmanta.ast.type.TYPES
maps attribute type string to inmanta types, but this is subtly different from the type mapping at the plugin boundary. E.g."dict": LiteralDict(),
Vsdict: inmanta_type.TypedDict(inmanta_type.Type()),
And this is a pre-existing conditionThere 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.
Ok, clear!
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
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'm not a fan of this fallback to
Any
. This may easily give the user the false impression that they're getting type validation, while that is not actually the case.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.
See my proposed extensions on slack
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.
Ok, so for iso 8.1 we also aim to evaluate strings here, right?
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 had understood that every string is considered to be an inmanta type
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 I would prefer to also support python type strings, because that's what you would expect when writing Python. But that doesn't have to be now necessarily. What I had in mind was to
If 1 fails -> fall back to dsl type parsing.
But I don't feel too strongly 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.
I don't want that. That would create the same type of problem that yaml has (is
00:00:00:00
a number?), that parsing outcome depends on context you (often) can't know. if a type 'string' is somehow in scope, the typing changesThere 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.
Perhaps a key element in my reasoning is that I approach it from the idea that the pure dsl annotations would eventually disappear, leaving only the
typing.Annotated
when you really need them. With that reasoning, the Python parsing would always be the primary, and the DSL parsing would become only a backwards compatibility fallback.But if that's not how you see it evolve, then perhaps I agree with you, given your argument above. I still think it can have unintuitive aspects of its own, but the
typing.Annotated
should give us a way to work around any we encounter to give ourselves time to discover where we really want to go.