serenity
serenity copied to clipboard
`&mut` methods on builders
It's nice to have the ability to pass the builder around as &mut builder
Could you elaborate on what you mean by this? Are you referring to the recent builder pattern overhaul, or something else?
Yes! On the next branch it is,
This was a decision I made to allow for builders to be constructed inline, for example:
let builder = CreateChannel::default().name("test-channel").topic("example topic");
guild_id.create_channel(&http, builder).await?;
instead of
let mut builder = CreateChannel::default();
builder.name("test-channel");
builder.topic("example topic");
guild_id.create_channel(&http, builder).await?;
And yes, that means that if you're adding fields conditionally or at some later point, instead of just calling builder.method(), you'll have to call builder = builder.method(). But, I think the changed ergonomics suit the new paradigm best.
Is there a reason to not have those &mut methods at all? Like we can name those &mut methods with set_ prefix, for example:
let mut builder = CreateEmbed::default().title("Foo");
if msg.content == "bar" {
builder.set_description("bar");
}
msg.channel_id.send_message(&http, CreateMessage::default().embed(builder));
This is just my opinion but using builder = builder.description("bar"); instead makes me feel a bit awkward and uncomfortable
Sorry for the delayed response. The main reason is to avoid polluting every builder type with extra methods. Adding a set_field method to each builder, and for every one of that builder's fields, would double the number of methods for that type and would really only serve to confuse people when reading docs. I'm aware there's a schism between people who prefer the &mut Self builder pattern vs. the Self builder pattern, but mixing the two is definitely bad news. Unfortunately that means some (likely less common) use cases suffer a hit to ergonomics.
I agree; there's a trade-off here, but we're better off not introducing an extra set of methods with &mut self
There's also options like https://docs.rs/replace_with
fn foo(builder: &mut CreateEmbed) {
replace_with_or_abort(builder, |b| b.name("test-channel"));
replace_with_or_abort(builder, |b| b.topic("example topic"));
}
Or macros
macro_rules! set {
($b:ident $( $rest:tt )*) => { $b = $b $( $rest)* };
}
fn foo(mut builder: CreateEmbed) -> CreateEmbed {
set!(builder.name("test-channel"));
set!(builder.topic("example topic"));
}
(disclaimer: both untested)
Anyhow, I don't think we'll be able to come to more of a resolution than we already have, so I believe this issue can be closed?