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

feature: add an opt version of the builders when strip_option is enabled. #117

Open
sosthene-nitrokey opened this issue Sep 19, 2023 · 9 comments

Comments

@sosthene-nitrokey
Copy link

Hi!

Currently strip_option prevents the actual setting of None, forcing to use two code paths when one wants to maybe build with None.
I propose the following. When using strip_option, a second setter is added, with an _opt suffix, that takes an option.

Here is an example:

Current version

#[derive(TypedBuilder)]
struct SomeStruct {
    #[builder(default, setter(strip_option)]
     inner: Option<u32>,
}

fn some_fn(value: Option<u32>) -> SomeStruct {
    match value {
        None => SomeStruct::builder().build(),
        Some(v) => SomeStruct::builder().inner(v).build(),
    }
}

Proposal:

#[derive(TypedBuilder)]
struct SomeStruct {
    #[builder(default, setter(strip_option)]
     inner: Option<u32>,
}

/// The old way still works the same
fn old_way(value: Option<u32>) -> SomeStruct {
    match value {
        None => SomeStruct::builder().build(),
        Some(v) => SomeStruct::builder().inner(v).build(),
    }
}

fn new_way(value: Option<u32>) -> SomeStruct {
    /// There are now 2 ways to set `inner`, allowing both uses, but privileging the `Some` case as intended.
    SomeStruct::builder().inner_opt(value).build()
}

Limitations

This would be a breaking change, since it would break the compilation of structures having for example an inner and an inner_opt field. This could be made opt-in for example by requiring the user to give a name to the _opt version.

@mo8it
Copy link
Contributor

mo8it commented Sep 20, 2023

I think opt-in is a good idea. Something like setter(strip_option, option_method)

@idanarye
Copy link
Owner

I'd like to combine this with #119 - I think a combined solution will be both simpler and more flexible.

Semantically, this means that inner_opt (or opt_inner? Does Rust have a convention for this?) will not set the argument as is. Instead, it'd look something like this:

fn inner_opt(self, value: Option<u32>) -> BuilderWithInnerSet {
    if let Some(value) = value {
        self.inner(value) // this will wrap with the Option internally
    } else {
        self.inner(predefined_default())
    }
}

The predefined_default(), BTW, does not have to be the regular default. We can set it separately in the option_method settings:

#[builder(default = Some(5), setter(strip_option, option_method(default = None)))]
inner: Option<u32>,

Or maybe even not have a regular default at all:

#[builder(setter(strip_option, option_method(default = None)))]
inner: Option<u32>,

Which will force using either inner_opt() or inner().

@idanarye
Copy link
Owner

BTW - I think we should bikeshed the names of these settings:

  • Can we find something more descriptive option_method?
  • Does it make sense to use default inside option_method? Maybe fallback would be better?

@dimitrilw
Copy link

dimitrilw commented Sep 22, 2023

Everybody loves scope creep!

My feature request (#119) is adjacent to this request (as noted by idanarye). Essentially, I would really like to have conditional logic when setting Struct fields during the builder.

Right now, I can achieve this via deriving Clone, declaring a mutable struct, making conditional changes, and then cloning into immutable. Here is some Rust(ish) psuedo-code:

use std::clone::Clone;
use typed_builder::TypedBuilder;

#[derive(Clone, Debug, TypedBuilder)]
struct Something {
    b: bool,

    #[builder(default=99)]
    i: i32,

    #[builder(default="hello from Something's default string".to_string())]
    s: String,
}

fn main() {
    let mut s = Something::builder().b(true).build();
    println!("{:?}", s);
    // output = Something { b: true, i: 99, s: "hello from Something's default string" }

    // this is a contrived example, but it shows the point
    // ...more real-world would be like "if config.i > 0 { s.i = config.i }"
    if true { s.i = 123; }
    // and of course, this block does nothing; default s value is retained
    if false { s.s = "changed s value from inside conditional".to_string(); }
    println!("{:?}", s);
    // output = Something { b: true, i: 123, s: "hello from Something's default string" }

    let s = s.clone();
    // s.s = "s, after clone".to_string(); // compile error! ...as desired; it is immutable again
    println!("{:?}", s);
    // output = Something { b: true, i: 123, s: "hello from Something's default string" }
}

And it would be really nice to perform those conditionals without using Clone; perhaps something like:

use typed_builder::TypedBuilder;

#[derive(TypedBuilder)]
struct Something {
    b: bool,

    #[builder(default=99)]
    i: i32,

    #[builder(default="hello from Something's default string".to_string())]
    s: String,
}

fn main() {
    let s = Something::builder()
        .b(true)
        .i_if(if true { 123 })
        .s_if(if vec![1,2,3].len() > 2 {
                "vec's len is larger than 2".to_string()
            } else if some_var == "meh" {
                "blah blah".to_string()
            })
        .build();
    
    // s.s = "cannot change because it is immutable".to_string(); // compile error! ...as desired
}

...and I'm not advocating for VAR_if as a naming convention. Just used it here to really clearly illustrate the conditional nature of that builder method.

In my hand-jammed Builder pattern Structs (which are far less feature-rich than TypedBuilder), I've achieved this capability via adding a method (that I have been naming "map"), something like:

impl Something {
    // various setter functions: fn s(), etc
    
    fn map<F>(&self, f: F) -> Something
    where F: Fn(&Something) -> Something {
        f(&self)
    }
}

Something
    .b(true)
    .map(|builder| {
             if true { 
                 builder.i(123) 
             } else { 
                 builder 
             }
         })
    .map(|builder| {
            if vec![1,2,3].len() > 2 {
                builder.s("vec's len is larger than 2".to_string())
            } else if some_var == "meh" {
                builder.s("blah blah".to_string())
            } else {
                builder
            }
        })
    .build();

@idanarye
Copy link
Owner

Right now, I can achieve this via deriving Clone, declaring a mutable struct, making conditional changes, and then cloning into immutable.

Protip: you don't actually need the Clone. You can just do this:

let s = s;

This will move the value of s to a new binding - also named s. The new binding will be immutable because it was not declared with mut. The old binding will be inaccessible for two reasons:

  1. Rust's move semantics. Something is not Copy, so after using a "consuming" operation - like assigning it - any code that runs afterwards cannot access it.
  2. Shadowing. You can't access the old s because the new s occupies its name.

Another option is to utilize the fact that blocks are expressions:

let s = {
    let mut s = Something::builder().b(true).build();
    println!("{:?}", s);
    // output = Something { b: true, i: 99, s: "hello from Something's default string" }

    // this is a contrived example, but it shows the point
    // ...more real-world would be like "if config.i > 0 { s.i = config.i }"
    if true { s.i = 123; }
    // and of course, this block does nothing; default s value is retained
    if false { s.s = "changed s value from inside conditional".to_string(); }
    println!("{:?}", s);
    // output = Something { b: true, i: 123, s: "hello from Something's default string" }

    s
};

@dimitrilw
Copy link

🤯
I have so much to learn about Rust. :) .....as you may have gathered, I'm still super new to this language. But it isn't my first. I really need to back-up, read The Book, and get my basic foundation solidified a bit.

I'm really fond of the let s = { /* stuff */ }; syntax -- very readable.

Thank you for sharing those suggestions! 😄

@lpotthast
Copy link

I would also love to have the flexibility of this request. Using strip_option really limits your users, which I would really like to avoid to in my case. (The same case can be opened for strip_bool!)

When your users only have Options at their hand themselves, passing them through to the builder pattern is extremely cumbersome, as you would have to break out of the typical builder-call-chain and add ifs around any option-stripped builder function.

Is there any progress on this? Can we help implement the feature?

@ifiokjr
Copy link

ifiokjr commented Jun 19, 2024

Would the following API be suitable?

#[derive(TypedBuilder)]
struct SomeStruct {
     #[builder(default, setter(strip_option)]
     inner: Option<u32>,
     #[builder(setter(strip_option="outer_fixed")]
     outer: Option<u32>,

}

/// `strip_option` retains its current behaviour avoiding a breaking change.
/// `strip_option="outer_fixed"` creates a new builder method which strips the `Option`. 
/// This makes it fully opt-in.
fn new_api(value: Option<u32>) -> SomeStruct {
    match value {
        None => SomeStruct::builder().outer(None).build(),
        Some(v) => SomeStruct::builder().inner(v).outer_fixed(v).build(),
    }
}

/// `strip_option="outer_fixed" retains the original `outer` builder method which still takes an `Option`.
fn new_way(value: Option<u32>) -> SomeStruct {
    SomeStruct::builder().outer(value).build()
}

Now rather than appending _opt which would be a breaking change, we add the ability to name the builder method which will have the Option stripped.

I can create a PR if this seems suitable @sosthene-nitrokey.

@sosthene-nitrokey
Copy link
Author

I would rather have it working the other way around. When using this it would be generally because the case with the option stripped is the "default", which you want to have the simplest name:

#[derive(TypedBuilder)]
struct SomeStruct {
     #[builder(default, setter(strip_option)]
     inner: Option<u32>,
     #[builder(setter(strip_option(fallback="outer_opt"))]
     outer: Option<u32>,

}

/// `strip_option` retains its current behaviour avoiding a breaking change.
/// `strip_option(fallback ="outer_opt")` strips the `Option`, and creates a fallback that still takes the option. 
/// This makes it fully opt-in.
fn new_api(value: Option<u32>) -> SomeStruct {
    match value {
        None => SomeStruct::builder().outer_opt(None).build(),
        Some(v) => SomeStruct::builder().inner(v).outer(v).build(),
    }
}

/// `strip_option(fallback ="outer_opt") makes the original `outer` builder method take the value, and creates an `outer_opt` which still takes an `Option`.
fn new_way(value: Option<u32>) -> SomeStruct {
    SomeStruct::builder().outer_opt(value).build()
}

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

No branches or pull requests

6 participants