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

Add an option to extend collection-like fields #196

Closed
wants to merge 1 commit into from

Conversation

andy128k
Copy link
Contributor

Addresses #114

Copy link
Collaborator

@TedDriggs TedDriggs left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, and great work on the code. Do we have examples of analogous builder APIs from crates that hand-implement their builders? The syntax for this feels a bit strange, but that might just be that it's new to me.

@andy128k
Copy link
Contributor Author

andy128k commented Feb 11, 2021

@TedDriggs I am not aware of such crates. I agree that name of setter <field>_extend_one is bumpy (it follows std). But guessing singular name of a setter from the plural name of a field and do it properly (English is hard :)) will take much more code than this feature deserves.

  • <field>_append or append_<field> look better but semantically incorrect for unordered collections (e.g. HashMap)
  • <field>_insert or insert_<field> are too broad (what is an insert position?)
  • <field>_add or add_<field> are too Java-ish 😏
  • Here my English vocabulary reaches a hardcover.

@TedDriggs
Copy link
Collaborator

Here my English vocabulary reaches a hardcover.

Your vocabulary is totally up to the task; the challenge here is that the underlying concept is more complex than a single word will accurately convey.

In the original issue, one proposal is that for some field example, calling builder.example(...) would push/extend/add an item, while builder.examples(...) would overwrite the whole collection. What if the declaration looked like this:

pub struct MyStruct {
    #[builder(setter(multiple(replace = "policies")))]
    policy: Vec<Policy>,
}

This would then generate

pub struct MyStructBuilder {
    policy: Option<Vec<Policy>>,
}

pub struct MyStructBuilder {
    policy: Option<Vec<Policy>>,
}

impl MyStructBuilder {
    pub fn policy(&mut self, value: Policy) -> &mut Self {
        self.policy
            .get_or_insert_with(|| Default::default())
            .extend(::std::iter::once(policy));
        self
    }

    pub fn policies(&mut self, value: Vec<Policy>) -> &mut Self {
        self.policy = Some(value);
        self
    }
}

The second setter is only emitted if replace = "..." is present in the setter's declaration. The main advantage of this approach is that the names stay concise, and each additional setter is opt-in:

  • The simplest builder attribute provides the simplest behavior
  • Declaring #[builder(setter(multiple))] changes the setter using the extend approach you've shown here, but doesn't force a second setter
  • Declaring #[builder(setter(multiple(replace = "...")))] does create both setters, and puts the burden on the deriver to choose a good name.

Thoughts?

@andy128k
Copy link
Contributor Author

@TedDriggs What if a field is named in plural form? In the case of #[builder(setter(multiple))] a setter will be fn policies(&mut self, value: Policy)? This looks strange.

What about #[builder(setter(multiple(one = "policy", all = "policies")))]? And if names are not given (#[builder(setter(multiple))] then fall back to <field_name> for all and <field_name>_add for one.

@ShaneEverittM
Copy link

ShaneEverittM commented Mar 15, 2021

In David Tolnay's proc-macro workshop, he uses the syntax: #[builder(each = "name")] above the field to be extended, which will generate a function called "name" that extends the collection. Would love it if this PR was merged.

@andy128k
Copy link
Contributor Author

I like each = "name" and created an alternative PR #199

@TedDriggs
Copy link
Collaborator

Closing in favor of #199

@TedDriggs TedDriggs closed this Mar 17, 2021
@andy128k andy128k deleted the issue_114 branch March 19, 2021 19:58
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.

None yet

3 participants