-
Notifications
You must be signed in to change notification settings - Fork 914
allow constructor customization of complex enum variants #4158
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
Conversation
|
I like this idea! I'll try to review the full code tomorrow, though I have some initial thoughts about how this extends:
|
Yeah, that came up in #4109 (comment) too. I think that extension is a good argument to go with a more descriptive keyword for that context.
Hmm, interesting idea. It might be pretty tricky to guarantee that the correct variant is constructed if we open this for users. |
Ah, very true, I guess we probably cannot, which is a good reason not to add that. And they can always add their own |
davidhewitt
left a comment
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.
This looks good to me! 👍
Just a few small suggestions, and this also needs documentation before it can be merged.
|
I've added to the complex enum section of the guide and put the attribute in the |
davidhewitt
left a comment
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 that's sufficient for now, thanks!
pyo3-macros-backend/src/pyclass.rs
Outdated
| Ok(quote! { #cls::#variant_name => #repr, }) | ||
| }) | ||
| .collect::<Result<TokenStream>>()?; |
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 collect can be made infallible again.
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.
Ah, yes. I reverted it to how it was before 👍
|
@davidhewitt, @Icxolu thank you for implementing this! |
This is an (experimental) idea I had to support customization of the macro generated constructors for complex enum variants.
This allow placing the the
#[pyo3(signature = ...)]attribute on complex enum variants and applies it to the generated constructor.This way all the normal functionality of signature customization can also be applied for these generated constructors.
What I'm not completely sure about is whether we should reuse the
signaturename here, or whether we should have have some kind of alias like#[pyo3(constructor = ...)]. On one hand people are already familiar with howsignatureworks, but using it on an enum variant (or anything that's not a function for that matter) is not so intuitive.Closes #4109