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 maybe_ prefix overrides at the top level with #[builder(on(...))] #230

Open
MuhannadAlrusayni opened this issue Dec 6, 2024 · 3 comments · May be fixed by #233
Open

Support maybe_ prefix overrides at the top level with #[builder(on(...))] #230

MuhannadAlrusayni opened this issue Dec 6, 2024 · 3 comments · May be fixed by #233
Labels
feature request A new feature is requested

Comments

@MuhannadAlrusayni
Copy link

MuhannadAlrusayni commented Dec 6, 2024

Hi there,

First off, thank you for creating and maintaining Bon—it’s been a pleasure working with it!

As I migrate a codebase to Bon, I’ve run into a challenge with the naming conventions for setters handling Option fields. My current codebase uses set_ as the prefix for these setters. While Bon's built-in support for Option setters is a fantastic feature that removes the need for manual definitions, it uses the maybe_ prefix by default.

This mismatch means I still have to manually rename every setter to match my convention, which undermines some of the convenience.

I’d like to suggest two potential improvements to make this process smoother for me and other users:

  1. Set set_ as the default prefix for Option setters (this align with naming in Introduce bool shorthand (#[builder(flag)]) #142)
  2. Introduce configurability for setter prefixes at the type level:
    Allowing users to define their preferred prefix (e.g., set_, maybe_, or custom ones) would offer greater flexibility and make Bon more adaptable to different codebases and conventions.

While either of the suggested changes would address my use case, I believe adding the ability to configure this at the type level would provide greater flexibility and cover a wider range of use cases.

Code snippet:

#[derive(Builder)]
#[builder(on(optional, prefix = "set_"))] // Example syntax for explanation, not necessarily the suggested approach
struct Foo {
    value: Option<String>,
    label: Option<String>,
}

A note for the community from the maintainers

Please vote on this issue by adding a 👍 reaction to help the maintainers with prioritizing it. You may add a comment describing your real use case related to this issue for us to better understand the problem domain.

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 6, 2024

Hi, thank you for creating the issue! Overriding the maybe_ prefix is indeed something that I've been thinking of having in bon.

I suppose as a workaround you use #[builder(setters(option_fn(name = set_{member_name})))]? That's indeed rather inconvenient. Having a prefix supported via #[builder(on(...))] would be preferred.

I have a big and complex issue about non-flag attributes support in #[builder(on(...))] (#152) with a comprehensive design for precedence and conflicts. However, I don't think this feature would have to be blocked by the full implementation of #152, because I believe overriding the maybe_ prefix may be a frequent use case. We can have a limited, but still useful implementation that accepts only this syntax:

#[builder(on(_, setters(option_fn(prefix = set_))))]

This would apply the setters(option_fn(prefix))) config for every default/optional members (the obviously knows it can skip this config for required members, so we can just use _ pattern here). Notice that the config here is exactly setters(option_fn(prefix)), to make it clear that the prefix only pertains to option_fn, otherwise users might confuse it with the prefix for a non-None setter (codename: some_fn), or for getters (#225) or for the name of the member in the typestate.

@Veetaha Veetaha added the feature request A new feature is requested label Dec 6, 2024
@Veetaha Veetaha changed the title Add the ability to change maybe_ for optional fields Support maybe_ prefix overrides at the top level with #[builder(on(...))] Dec 6, 2024
@MuhannadAlrusayni
Copy link
Author

I suppose as a workaround you use #[builder(setters(option_fn(name = set_{member_name})))]? That's indeed rather inconvenient. Having a prefix supported via #[builder(on(...))] would be preferred.

This is what I am doing, and yeah it's inconvenient to do that for a lot of fields in a lot of types.

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 12, 2024

@MuhannadAlrusayni I'd like to clarify something more.

What is the exact desired naming pattern in your case? For example, the current naming pattern in bon is the following:

#[derive(bon::Builder)]
struct Example {
    required: u32,
    optional: Option<u32>
}

Example::builder()
    // A single setter for the required field without any prefix
    .required(1)
    // Two setters for optional fields. This one accepts a non-None value without any prefix.
    // This is an intentional design choice, so that changing `u32 -> Option<u32>` (i.e. making
    // the required field optional) is fully compatible.
    .optional(2)
    // The other setter for the optional field, that accepts an `Option<u32>` directly.
    .maybe_optional(Some(3));

As I understand you would like to have the following instead?

Example::builder()
    .required(1) // same
    .optional(2) // same
    .set_optional(Some(3)) // uses `set_` prefix instead of `maybe_`

Please confirm if I understood you correctly in the snippet above.

Attribute Config Design

I thought a bit more about the design for the attributes config. I think, it makes sense to expose a configurable prefix at several levels.

I envision the following attributes design:

#[derive(bon::Builder)]
struct Example {
    #[builder(setters(prefix = foo_))]
    required: u32,
    
    #[builder(setters(prefix = bar_))]
    optional_1: Option<u32>,
    
    #[builder(setters(option_fn(prefix = baz_), some_fn(prefix = qux_)))]
    optional_2: Option<u32>,
}

Example::builder()
    .foo_required(1)
    // Setters for `optional_1`
    .bar_optional_1(2)
    .bar_maybe_optional_1(Some(3))
    // Setters for `optional_2`
    .baz_optional_2(4)
    .qux_optional_2(Some(5));

I think the prefix can be used in combination with different name overrides like this:

#[derive(bon::Builder)]
struct Example {
    // Produces `bar_foo(T)` and `bar_maybe_foo(Option<T>)` setters
    #[builder(name = foo, setters(prefix = bar_))]
    optional: Option<u32>,

    // Produces `bar_foo(T)` and `bar_maybe_foo(Option<T>)` setters too.
    // The difference here is that `setters(name)` doesn't influence
    // the name of the member in the builder's type state and `#[builder(getter)]`
    #[builder(setters(name = foo, prefix = bar_))]
    optional: Option<u32>,

    // Produces `bar_foo(T)` and `qux_baz(Option<T>)` setters
    #[builder(setters(
        option_fn(name = foo, prefix = bar_),
        some_fn(name = baz, prefix = qux_)
    ))]
    optional: Option<u32>,
}

And then there will also be a way to specify these same parameters with #[builder(on(_, ...))] at the top level:

// Applies a given `prefix` for all setters (ones that accept `T` and ones that accept `Option<T>`)
#[builder(on(_, setters(prefix = bar_)))]

// Applies a given `prefix` granularly: `bar_` prefix for optional setters that accept `T`
// and `qux_` prefix for optional setters that accept `Option<T>`
#[builder(on(_, setters(option_fn(prefix = bar_), some_fn(prefix = qux_)))]

Unfortunatelly, the syntax for your use case (where you'd like to override the prefix only for option_fn if I understood you correctly), would be a bit more verbose (#[builder(on(_, setters(option_fn(prefix = set_))))), but otherwise I think this would be a more intuitive API. You could further shorten that syntax by using some of the suggestions from this page.

@Veetaha Veetaha linked a pull request Dec 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A new feature is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants