Ergonomics of `VorbisEncoderBuilder`

[astral4]

The way the builder pattern is used for VorbisEncoderBuilder makes constructing a VorbisEncoder slightly awkward. Here's an example of how to go about it currently:

#[allow(unused_results)]

let mut builder = VorbisEncoderBuilder::new(sample_rate, channels, sink)?;

builder.bitrate_management_strategy(VorbisBitrateManagementStrategy::QualityVbr {
    target_quality: 1.0,
});

let encoder = builder.build()?;

I think this would be better:

let encoder = VorbisEncoderBuilder::new(sample_rate, channels, sink)?
    .bitrate_management_strategy(VorbisBitrateManagementStrategy::QualityVbr {
        target_quality: 1.0,
    })
    .build()?;

Methods on builders usually go like this (see the Design Patterns book and reqwest::ClientBuilder for examples):

pub fn foo(mut self, bar: ...) -> Self

but for VorbisEncoderBuilder, the methods look like this:

pub fn foo(&mut self, bar: ...) -> &mut Self

The issue is that VorbisEncoderBuilder::build consumes self when the methods return &mut Self, so you can't chain build with them. I'm also not sure why methods return &mut Self at all. If a user has ownership of the builder struct, getting a &mut Self is trivial. Another smaller issue is that not using the return type (like in the example) triggers the unused_result lint. This can be dealt with, but the workaround should not be needed in the first place.

[AlexTMjugador]

Hi, thanks for sharing your suggestion! :heart:

My inspiration for the design of VorbisEncoderBuilder came from tokio::runtime::Builder, which as you can see uses &mut self method receivers. I think that using mutable reference receivers is more semantic than owning receivers, as they conceptually mutate the state of the builder rather than destroying it and creating a new, updated one. By reference receivers also support advanced use cases involving sharing the builder between threads, RefCell, and the like.

However, I agree with your point that it is unergonomic for VorbisEncoderBuilder::build to have an owning receiver if other methods don't. I didn't think that through, and there is no technical reason for it. Do you agree that it'd be better if VorbisEncoderBuilder::build also had a &mut self receiver?

[astral4]

Having VorbisEncoderBuilder::build accept &mut self would definitely work. Since the builder is no longer consumed, it can still be used after a VorbisEncoder is made. Semantically, I'm not sure keeping the builder around makes sense, but it might be fine if someone wanted to create multiple encoders from the same builder. Regardless, this change would resolve this issue. Thanks for your quick response!

[AlexTMjugador]

[...] but it might be fine if someone wanted to create multiple encoders from the same builder.

Indeed, this is a use case I would like to support, as it's reasonable for batch encoding applications to benefit from reusing builders.

Thanks again for your input. I'm closing this issue and scheduling the change I described for the next breaking release, but please feel free to get in touch if you have anything else to share!