[feature request] supporting multiple enum variants about fancy_constructor-rs HOT 9 OPEN

@OmarTawfik Just came back to this with a fresh mind 😄

There's a bit of a conflict of interest going on - I'm all for introducing new ways to construct variants, but I'm strongly against making deriving new() on enums any less convenient than it is now: new() is and will continue to be the primary focus of this library and any additional functionality should only compliment that, hence my earlier suggestions leaning more on side-by-side approaches instead of full on replacements (not needing a breaking change is just a nice side-effect). Looking over our chat, it's entirely my bad for not mentioning this from the start and jumping straight to brainstorming - my apologies for that.

With that said and given how new() enums and enums with variant constructors would have minor differences in options (constructor options becoming variant options, the old #[new] variant marker no longer being needed), I really think a second macro export with a different attribute name (so it doesn't conflict with #[new] in case someone wants to derive both constructor styles) would result in the cleanest API overall.

Since all the option names would be the same, there shouldn't be any learning curve involved for users discovering per-variant constructors and, since the config attribute name would be different, there should be no confusion re: which macro is getting configured.

from fancy_constructor-rs.

Hey, I'm afraid I won't have capacity for this in the near future, so I can't give an implementation timeline

from fancy_constructor-rs.

@Alorel Not sure about my schedule either, but would you accept a PR for it later?

If yes, please let me know how you think we can consolidate my suggestion about with the current implementation.
Would the new capability/feature justify a breaking change/major version bump?

Thanks for considering!

from fancy_constructor-rs.

@OmarTawfik A PR would be very welcome, thanks! I'd like to avoid a breaking version release if possible, but don't have too strong a preference - if it's needed, it's needed (although I think you should be able to get away without a breaking release - multiple variants aren't supported right now and this'd be adding support so more likely than not it'd just be a feature release).

As for implementation, I think the simplest approach would be to add a new variant to the library's internal FieldsSource enum for handling enums with multiple fields and handle it there - if there's one #[new] variant, use FieldsSource::Enum like the library does now, if there's multiple, use your new implementation. For each variant marked, parse its attributes for, I guess, ContainerOptions (I think the same options that apply to structs apply to individual variant constructor fns?). Add a bit of validation - don't allow ContainerOptions to be provided if FieldsSource::Enum is being parsed.

Sound about right?

EDIT: and, I guess, another bit of validation would be needed - if FieldsSource::Enum is used, ContainerOptions would go on the enum, if FieldsSource::SuggestedImplementation is used, it'd go on the variant.

from fancy_constructor-rs.

@Alorel

Thank you for the tips. I think I will have more time after the holidays.

I think there is a difference between both here, about the name of the constructor method. For structs it is fine to generate fn new() -> Self because there is only one. For enums, we would have to use the name of each variant fn variant() -> Self. Do you think switching between both syntaxes might be confusing to users? especially that overrides via #[new(name(XXX))] are also possible.

To prevent this confusion, I was suggesting to deprecate putting the empty #[new] on variants, and always generate fn variant() -> Self for each. I think the consistency here might justify the breaking change, as I expect most users to benefit from the new behaviour, and we can even put an error message on the variant-specific #[new] that says something like below, to help users with the upgrade:

In version XXX, constructors are generated for all variants by default, so annotating a single variant is no longer needed.

The alternative to avoiding the breaking change is to support both at the same time? Always emitting all the variant-specific constructors, and if a variant has #[new], we would also generate fn new() -> Self for it. Multiple variants with #[new] would still be illegal. However, I think having both new() and variant() for the same variant would introduce even more confusion?

Thoughts?

from fancy_constructor-rs.

How about we keep the current behaviour unless the user explicitly enables yours? E.g. require something like a variant_constructors: bool option on the enum itself which would swap from new() to a constructor per variant.

That way we add a new piece of functionality without introducing a breaking change and consequently demanding more of library users' attention than blindly updating from 1.x to 1.y.

from fancy_constructor-rs.

@OmarTawfik apologies for the long delay, life got a bit hectic this time of year 😅

from fancy_constructor-rs.

Alternatively, I'm tempted to just export a new macro for it, e.g. variant_constructors? It might need a little bit of code restructuring but a lot of the groundwork is already done e.g. attribute options are all handled through the AttributeOptions macro and are trivial to create if different ones are needed, parsing is already largely separated per input type (enum/struct), tokenisation might just need more things extracted into separate functions and/or structs implementing ToTokens for optimal reuse

from fancy_constructor-rs.

apologies for the long delay, life got a bit hectic this time of year 😅

No worries at all 😄 same here!

Taking a look at the source code, and the ideas you suggested above, my preference would be to publish a new major version, where we use a single macro for both structs and enums, and generate fn variant_name() -> Self for all variants by default:

• This behaviour is highly desirable, and I think it would be the expected default for new users, especially coming from less configurable alternatives like derive_new or derived crates.
• It is cheaper to do the breaking change now, since fancy_constructor is relatively new, and only has one dependent so far, so we can easily upgrade it.
• Users can still maintain the legacy behaviour if they want, as they can still change the name of the function generated using the name(new) field if they want.
• We can even support a skip field on variants to not generate a function for it.

If not possible, and we cannot publish a new major version, then I think the only possible alternative is a new variant_constructors macro like you suggested above, although I really like the uniformity of using a single new macro for both structs and enums.

I will leave that decision up to you as the long-term maintainer. Thanks for brainstorming with me!

from fancy_constructor-rs.