Closed sosthene-nitrokey closed 2 weeks ago
I think opt-in is a good idea. Something like setter(strip_option, option_method)
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()
.
BTW - I think we should bikeshed the names of these settings:
option_method
?default
inside option_method
? Maybe fallback
would be better?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();
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:
Something
is not Copy
, so after using a "consuming" operation - like assigning it - any code that runs afterwards cannot access it.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
};
🤯 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! 😄
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 Option
s 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 if
s around any option-stripped builder function.
Is there any progress on this? Can we help implement the feature?
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.
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()
}
@idanarye / @sosthene-nitrokey I've created the PR in #150. I'd love to use the functionality in my current project. Can it be reviewed?
Hi!
Currently
strip_option
prevents the actual setting ofNone
, forcing to use two code paths when one wants to maybe build withNone
. I propose the following. When usingstrip_option
, a second setter is added, with an_opt
suffix, that takes an option.Here is an example:
Current version
Proposal:
Limitations
This would be a breaking change, since it would break the compilation of structures having for example an
inner
and aninner_opt
field. This could be made opt-in for example by requiring the user to give a name to the_opt
version.