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

allow deriving of FromVariant for uninhabitable enums #962

Merged

Conversation

Bogay
Copy link
Contributor

@Bogay Bogay commented Oct 13, 2022

Fixs #444
What type of error should be returned might need further discussion.

@Bogay Bogay marked this pull request as draft October 13, 2022 20:38
@Bromeon
Copy link
Member

Bromeon commented Oct 13, 2022

Thanks a lot for this PR! 👍

[Edit]: discussion about usefulness is starting from here.

bors try

bors bot added a commit that referenced this pull request Oct 13, 2022
@Bromeon Bromeon added feature Adds functionality to the library c: export Component: export (mod export, derive) labels Oct 13, 2022
@bors
Copy link
Contributor

bors bot commented Oct 13, 2022

try

Build succeeded:

@Bromeon Bromeon linked an issue Oct 14, 2022 that may be closed by this pull request
chitoyuu added a commit to chitoyuu/godot-rust that referenced this pull request Oct 18, 2022
Adds the `#[variant(enum = "repr")]` and `#[variant(enum = "str")]`
item-level attribute to the Variant conversion derive macros for
fieldless enums.

- `#[variant(enum = "repr")]` - convert through the numeric type
  specified using `#[repr]`.
- `#[variant(enum = "str")]` - convert through string representations of
  the variant names.

WIP - do not merge! FromVariant implementations would conflict
with godot-rust#962.

Close godot-rust#546.
chitoyuu added a commit to chitoyuu/godot-rust that referenced this pull request Oct 18, 2022
Adds the `#[variant(enum = "repr")]` and `#[variant(enum = "str")]`
item-level attribute to the Variant conversion derive macros for
fieldless enums.

- `#[variant(enum = "repr")]` - convert through the numeric type
  specified using `#[repr]`.
- `#[variant(enum = "str")]` - convert through string representations of
  the variant names.

WIP - do not merge! FromVariant implementations would conflict
with godot-rust#962.

Close godot-rust#546.
chitoyuu added a commit to chitoyuu/godot-rust that referenced this pull request Oct 18, 2022
Adds the `#[variant(enum = "repr")]` and `#[variant(enum = "str")]`
item-level attribute to the Variant conversion derive macros for
fieldless enums.

- `#[variant(enum = "repr")]` - convert through the numeric type
  specified using `#[repr]`.
- `#[variant(enum = "str")]` - convert through string representations of
  the variant names.

WIP - do not merge! FromVariant implementations would conflict
with godot-rust#962.

Close godot-rust#546.
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Just one minor comment.

I think FromVariantError::UnknownEnumVariant might be good for now? We can still change this post-PR.

Comment on lines -65 to -103
{
let __dict = ::gdnative::core_types::Dictionary::from_variant(#input_ident)
.map_err(|__err| FVE::InvalidEnumRepr {
expected: VariantEnumRepr::ExternallyTagged,
error: std::boxed::Box::new(__err),
})?;

let __keys = __dict.keys();
if __keys.len() != 1 {
Err(FVE::InvalidEnumRepr {
expected: VariantEnumRepr::ExternallyTagged,
error: std::boxed::Box::new(FVE::InvalidLength {
expected: 1,
len: __keys.len() as usize,
}),
})
}
else {
let __key = String::from_variant(&__keys.get(0))
.map_err(|__err| FVE::InvalidEnumRepr {
expected: VariantEnumRepr::ExternallyTagged,
error: std::boxed::Box::new(__err),
})?;
match __key.as_str() {
#(
#ref_var_ident_string_literals => {
let #var_input_ident_iter = &__dict.get_or_nil(&__keys.get(0));
(#var_from_variants).map_err(|err| FVE::InvalidEnumVariant {
variant: #ref_var_ident_string_literals,
error: std::boxed::Box::new(err),
})
},
)*
variant => Err(FVE::UnknownEnumVariant {
variant: variant.to_string(),
expected: &[#(#ref_var_ident_string_literals),*],
}),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This {} block is no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's almost identical to L68 ~ L105. (expected the early_return statement)

There are so many lines of change due to indentation. The outermost {} pair is removed because return_expr is the only expression used inside generated from_variant implementation. Should I keep it?

https://github.com/godot-rust/godot-rust/blob/94d7a45672825c916bfd5425b1703a0db1a822a5/gdnative-derive/src/variant/from.rs#L68-L105

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks. No it's fine, you can leave your changes like this 🙂

@Bromeon Bromeon added this to the v0.11.x milestone Oct 18, 2022
@Bogay Bogay marked this pull request as ready for review October 18, 2022 10:23
@Bogay Bogay changed the title Draft: allow deriving of FromVariant for uninhabitable enums allow deriving of FromVariant for uninhabitable enums Oct 18, 2022
@Bromeon
Copy link
Member

Bromeon commented Oct 18, 2022

Thanks! 🚀

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 18, 2022

Build succeeded:

@bors bors bot merged commit d3924de into godot-rust:master Oct 18, 2022
@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.11.1 Dec 6, 2022
@Bogay Bogay deleted the allow-from-to-variant-for-uninhabitable-enum branch June 29, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow deriving of ToVariant and FromVariant for uninhabitable enums
3 participants