Skip to content
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

dynamic type function variants for Variants and Values #519

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Jan 31, 2022

Addresses the main part of #507 by providing explicit dynamic type variants of:

  • Variant::array_from_iter -> Variant::array_from_iter_with_type
    (a mouthful but consistent with other existing functions like from_data_with_type_trusted)
  • Variant::from_maybe becomes Variant::from_none and Variant::from_some
    • and adds an accessor: Variant::as_maybe
      (panicking, should there be a try_as_maybe? should it return an Option<Option<Variant>> to be more in line with Variant::as_variant?)
  • Variant::is -> Variant::is_type
    (consider is_of_type instead?)
  • Value::is -> Value::is_type (ditto)
  • Value::transform -> Value::transform_with_type
    (consider transform_into/transform_to_type instead?)

@sdroege
Copy link
Member

sdroege commented Jan 31, 2022

CC @cgwalters @lucab in case you have opinions

@sdroege sdroege linked an issue Jan 31, 2022 that may be closed by this pull request
sdroege
sdroege previously approved these changes Jan 31, 2022
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, these new methods looks useful to have 👍.

pub fn as_maybe(&self) -> Option<Variant> {
assert!(self.type_().is_maybe());

self.try_child_value(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have misunderstood the logic, but I'm not exactly sure that this matches what the docstring says.
Let's maybe cover this method with a test to avoid any doubts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A maybe variant is a container with either 1 or 0 values, thus roughly the logic for this function is if self.n_children() == 0 { None } else { Some(self.child_value(0)) } - Variant::try_child_value already exists as a rough equivalent to rust's slice.get(0) for this purpose, so it didn't seem worth duplicating the logic here.
However, I really should just use ffi::g_variant_get_maybe instead, since it's clearer - it implements the exact same logic regardless.

Good point on adding a test for this method though, will do!

Explicit dynamic type variants of `Variant::array_from_iter`,
`Variant::from_maybe`, `Variant::is`, `Value::is`, and
`Value::transform`
@sdroege sdroege merged commit 79e7a30 into gtk-rs:master Feb 1, 2022
@arcnmx arcnmx mentioned this pull request Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Variant constructors are too statically typed
3 participants