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

Support deriving builder for enums #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Nov 12, 2023

The name of builder methods are the "snake_case" of variant names. Note that it does not support enums with generics or lifetime.

The name of builder methods are the "snake_case" of variant names.
Note that it does not support enums with generics or lifetime.
@itchyny
Copy link
Contributor Author

itchyny commented Dec 10, 2023

Gentle ping, any thoughts on enum support?

Copy link
Owner

@idanarye idanarye left a comment

Choose a reason for hiding this comment

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

Right, sorry about that. I've actually started reviewing a few weeks ago, but things happened and I got distracted and left it half-reviewed...

/// `TypedBuilder` also supports deriving builder of enums.
/// By default (inconfigurable though), `TypedBuilder` generates builder methods with
/// the name of "snake_case" of variant names.
/// For example, the builder of `Foo::BarQux` is created by `Foo::bar_qux()`.
Copy link
Owner

Choose a reason for hiding this comment

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

It's common for enums to use these methods for returning an Option that's Some iff self's discriminant matches the method name (like Result::ok and Result::err from the standard library). Using the same name for the builder will conflict with these methods.

Wouldn't it be better to use Foo::bar_qux_builder()?

/// By default (inconfigurable though), `TypedBuilder` generates builder methods with
/// the name of "snake_case" of variant names.
/// For example, the builder of `Foo::BarQux` is created by `Foo::bar_qux()`.
/// Note that it does not support enums with generics or lifetime.
Copy link
Owner

Choose a reason for hiding this comment

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

Why? I mean, obviously you'd have to specify (if they cannot be inferred) the generics and lifetimes that are only relevant for the variants not chosen, but that is no different from regular Rust enums.

///
/// You can specify `#[builder(...)]` on enums, on variants, and on any fields.
/// But since `TypedBuilder` internally generates a builder struct each enum variant,
/// some subsections are prohibited; you cannot specify `builder_method(name=...)`,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this could be better worded? From the examples I understand that builder_method(name = ...), for example, goes on the variant and not on the top level type declaration, but from this sentence it seems like it's not allowed at all when using TypedBuilder on enums.

let builder_attr = TypeBuilderAttr::new(&ast.attrs)?;
if builder_attr.builder_method.name.is_some() {
return Err(Error::new_spanned(
ast,
Copy link
Owner

Choose a reason for hiding this comment

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

This should be the name, so that the error message will be able to point at it:

if let Some(name) = builder_attr.builder_method.name {
    return Err(Error::new_spanned(
        name,
        // ...

if builder_attr.builder_method.name.is_some() {
return Err(Error::new_spanned(
ast,
"TypedBuilder is not supported for enum with builder_method(name=...)",
Copy link
Owner

Choose a reason for hiding this comment

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

This should mention that this attribute can be placed on the variants.

if !matches!(builder_attr.build_method.into, IntoSetting::NoConversion) {
return Err(Error::new_spanned(
ast,
"TypedBuilder is not supported for enum with build_method(into=...)",
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto

use crate::builder_attr::{IntoSetting, TypeBuilderAttr};
use crate::struct_info::StructInfo;

pub struct EnumInfo<'a> {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this struct. Unlike StructInfo and FieldInfo, which hold a parsed representation of the struct/field ready to create the builder code from, this EnumInfo here holds direct references to the AST. It's new method does no parsing - only verification.

) -> syn::Result<TokenStream> {
let enum_name = &self.ast.ident;
let internal_struct_name = format_ident!("{}{}", enum_name, variant_name);
let internal_struct_ast = &syn::DeriveInput {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather create a StructInfo from the variant than fabricate a DeriveInput. Even if it means altering StructInfo::new to support this.

Copy link
Owner

Choose a reason for hiding this comment

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

Creating a StructInfo using the constructor is also acceptable here - most of the fields you need to fill are ones you manually fabricate in this DeriveInput anyway.

}

#[automatically_derived]
impl From<#internal_struct_name> for #enum_name {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like that we define a potentially visible internal struct just for this. I'd rather modify DeriveInput to have an Optional "parent enum" field, and have it directly generate the enum variant of that field.

@Veetaha Veetaha mentioned this pull request Jul 30, 2024
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.

2 participants