-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] Add Variant.is_type_supported
method.
#4057
base: main
Are you sure you want to change the base?
Conversation
c771a24
to
a43a1fd
Compare
mojo/docs/changelog.md
Outdated
Example: | ||
|
||
```mojo | ||
def MyFunction(mut arg: Variant): |
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.
Function name in Mojo should be snake_cased. I see also more reasons for having it as fn
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.
@rd4com, still relevant. Maybe we can also choose more self explanatory names for arg and func which illustrate usage example better than my_function
and arg
?
mojo/stdlib/src/utils/variant.mojo
Outdated
|
||
Example: | ||
```mojo | ||
def MyFunction(mut arg: Variant): |
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.
Same what above
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.
Can you help me understand a bit more about why this is useful?
Note that in C++, we have something similar: for checking if any of the non-active types match the asked type (https://en.cppreference.com/w/cpp/utility/variant/holds_alternative). In mp11, for type list, we have boost::mp11::mp_contains
if you're familiar with that.
It can be used in |
Hello, thanks for the reviews 👍 Here is the reasoning: If arg were If arg is just def MyFunction(mut arg: Variant) and we can take any Variant, like if we would: def MyFunction[*Ts: CollectionElement](mut arg: Variant[*Ts]) So from within that function, we have to check if the variant support a type, The usecase is pretty narrow, |
What's the case for naming function "MyFunction" (like struct) and not "my_function" and using
|
Yep, thanks for reviewing and working together to refine the pr @gryznar ❤️🔥 🤝 , |
Yeah, I see. Sorry for reminding, but this hurts my eyes 😂 |
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.
Hi, I understand the need for this functionality.
But maybe we can try and make it more obvious at first glance
fn can_be[T: CollectionElement]() -> Bool:
"""Check if a type can be stored inside the `Variant`.
But I think this would be even better once we have a mechanism to use __contains__
in parameter space:
if Int in Variant[Int, String]:
print("can be an Int")
it would only need VariadicPack
to have a __contains__
for types.
I'll play around with the idea and see if we have the type system features or if I manage to force it.
I like naming and future enhancment from @martinvuyk . In his example it is self explanatory |
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.
LGTM once the minor style comments are addressed. Ping me once they're addressed and I'm happy to sync this in and land it.
Thank you!
fn is_type_supported[T: CollectionElement]() -> Bool: | ||
"""Check if a type can be used by the `Variant`. | ||
|
||
Example: |
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.
Suggestion Move this after the API docs for parameter/returns sections for consistency with other Mojo code.
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.
Hello, implemented the suggestion 👍
But had to add:
For example, the
Variant[Int, Bool]
permitsInt
andBool
.
So that it ends with an .
👍
But it does convey too the idea that you suggested in the other review,
WDYT?
Hello, this commit adds a method to check if a `Variant` supports a type, for example: ```mojo def MyFunction(mut arg: Variant): if arg.is_type_supported[Float64](): arg = Float64(1.5) def main(): var x = Variant[Int, Float64](1) MyFunction(x) if x.isa[Float64](): print(x[Float64]) # 1.5 ``` Signed-off-by: rd4com <[email protected]>
Co-authored-by: Joe Loser <[email protected]> Signed-off-by: rd4com <[email protected]>
Co-authored-by: Joe Loser <[email protected]> Signed-off-by: rd4com <[email protected]>
Co-authored-by: Joe Loser <[email protected]> Signed-off-by: rd4com <[email protected]>
Co-authored-by: Joe Loser <[email protected]> Signed-off-by: rd4com <[email protected]>
Signed-off-by: rd4com <[email protected]>
7550737
to
fedd86f
Compare
Hello,
this commit adds a method to check if a
Variant
supports a type, for example: