serenity icon indicating copy to clipboard operation
serenity copied to clipboard

`&mut` methods on builders

Open tmokenc opened this issue 3 years ago • 7 comments

It's nice to have the ability to pass the builder around as &mut builder

tmokenc avatar Jul 25 '22 21:07 tmokenc

Could you elaborate on what you mean by this? Are you referring to the recent builder pattern overhaul, or something else?

mkrasnitski avatar Jul 25 '22 21:07 mkrasnitski

Yes! On the next branch it is,

tmokenc avatar Jul 25 '22 21:07 tmokenc

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.

mkrasnitski avatar Jul 25 '22 22:07 mkrasnitski

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

tmokenc avatar Jul 26 '22 09:07 tmokenc

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.

mkrasnitski avatar Aug 19 '22 03:08 mkrasnitski

I agree; there's a trade-off here, but we're better off not introducing an extra set of methods with &mut self

kangalio avatar Sep 10 '22 09:09 kangalio

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?

kangalio avatar Sep 18 '22 04:09 kangalio