Add Builder guidelines

[nagisa]

No description provided.

[reem]

Clone sure, but I'd remove Copy.

[killercup]

Rendered

[brson]

The guidelines may be a good place for this.

[nagisa]

This is guidelines for the standard library, not the overall ecosystem. This is also an RFC, because it has potential to make breaking changes to builders that already exist in the standard library.

[petrochenkov]

Could you list all the proposed changes to std in addition to just the guidelines? It is not obvious what changes are required because most of these conventions are already followed by the std builders.

OpenOptions::new().append(true).open("file.txt"); looks weird indeed.

[nagisa]

Sure, I’ve been thinking I should do that, but I won’t be able to get to it until weekend, sadly.

[alexcrichton]

I think that this finalizer convention actually needs to be the same as the above convention which is to say that Foo::new().bar().finalize() won't actually work if it consumes self (as bar() returned &mut self), perhaps this could recommend "a borrowed self" instead?

[nagisa]

Ahh, this is very disappointing, because moving would avoid copying owned data internally in most cases.

[alexcrichton]

This seems like an unfortunate ergonomic loss from read and write today on the OpenOptions builder. As a general guideline I think this is a great idea (aka name instead of named), but I think that this should be a "strives for" policy instead of hard-and-fast rule.

[nagisa]

This is a guideline and “should” does not mean “must” 😊

For all its worth, read and write both are nouns as well (but as I understand have an odd meaning to use them as a nouns). Added them to the list of examples as well.

[alexcrichton]

You can probably leave this part out actually as all types in Rust should strive to implement Clone!

[nagisa]

Seemed fine to include this as thread::Builder nor process::Command implement it.

[alexcrichton]

At this point it's pretty unlikely that we're going to accept this as a breaking change, but we're definitely open for a deprecation strategy. Would you be ok re-wording the relevant bits of this RFC to recommend a deprecation strategy instead of a breaking change?

[nagisa]

Sure!

[alexcrichton]

Could you add @sfackler's alternative here as well? I personally wouldn't advocate for it, but it seems like a legitimate alternative!

[alexcrichton]

Thanks for the RFC @nagisa! I definitely agree that this is an area that looks like it could use some better guidelines. I've left some specific comments with things that @aturon and I talked about, but I wanted to talk more broadly about two specific aspects:

• In terms of naming guidelines, it's a little unfortunate to lose Command which kinda "feels" like the right name for that API. I do, however, like the idea of putting "File" somewhere in the name of the file builder instead of calling it "OpenOptions", and "FileBuilder" does sound like a decent name. Using a name like just "Builder", however, precludes the possibility of having multiple builders in one module unfortunately. I'm wonder if in general this guideline could perhaps recommend FooBuilder if there's not a better name, Builder if you're sure there's only going to be one in the module?
• In terms of construction, there's a few vectors to go along as well. One is Builder::new(), another is TargetType::builder() (as @sfackler suggested), and a final one is perhaps builder_new() (a top-level function). I feel like @sfackler's suggestion may be hard to follow (for the thread::Builder-produces-two-things reason you brought up), and I would personally lean towards a ::new() constructor instead of a top-level function (but @aturon may feel differently). Just something to consider!

[nagisa]

I think I’ve addressed most of the feedback:

1. There’s now a section detailing exact changes proposed to the std Builders;
2. Fixed numerous mistakes and reworded the section on builder naming guidelines;
3. Updated alternatives 😉

[alexcrichton]

In the past I've been hesitant to do this as it may prevent us from adding more options to Command in the future. For example if a standard I/O handle is redirected to a file, it's not clear how Clone can be implemented.

We do have various routes to mitigate this, but this may fall under the category of "would like clone, but implementation may not make it possible".

[alexcrichton]

Currently this is done because the name is an owned value (e.g. scoped/spawn consumes it), but I do think that this is largely a holdover from the past where Box<Writer> or Box<FnOnce> was contained within and the by-value-ness was needed.

[nagisa]

Currently this is done because the name is an owned value

Yes, this is the primary reason why finalizers taking self by value would be great, if they didn’t introduce usability issues, as you pointed out in the previous revision.

[rkjnsn]

I think this adds helpful consistency, so +1. I would also be in support of renaming process::Command.

[lilyball]

A tentative 👍 here, but I'd rather keep process::Command. Besides the fact that the name just feels right (especially when it means I can use std::process::Command; and just refer to it as Command later), I'm also not convinced that the process module will never have a need for another builder. We've already added one function that has nothing to do with child processes (process::exit()) so I'd rather not prevent us from reasonably deciding that there's a second builder that makes sense to keep in the process module.

As a separate argument for the name, process::Command may use the builder pattern, but it does not build "process" values. The finalizers return Child, Output, or ExitStatus values. And these aren't "built" values either, they're the results of performing an external action described by the Command. The closest name that matches these conventions that I could see using is ChildBuilder, as what it's "building" is a child process, and the Output / ExitStatus finalizers are basically just convenience methods around Child. But even there, I still think the name Command is more evocative and ergonomic than process::ChildBuilder.

[BurntSushi]

I broadly like this RFC. For reasons that others have mentioned, I would prefer to keep the Command name. I do like the idea of replacing OpenOptions with FileBuilder.

One thing I'm confused about is this in the "alternatives" section:

Construct builders via a method implemented on the buildees. The API looks like this:

It seems to me like this could be a legitimate choice when designing builder-like things. I find that the reduction in the number of types to be a big advantage when possible. In particular, putting the builder on the buildee itself has worked well for me when there are a lot of optional parameters. e.g.,

let mut rdr = csv::Reader::from_file("foo.csv"); // sane defaults
let mut rdr = csv::Reader::from_file("foo.csv")
                          .delimiter('\t');      // tab delimited
let mut rdr = csv::Reader::from_file("foo.csv")
                          .has_headers(false)
                          .quote('\'')
                          .delimiter(';')
                          .escape(Some('\\'));   // something whacky

// any of the above `rdr`s can decode records.

It's true that this pattern can't be used for all things, but I'd find it unfortunate to rule it out just for that reason.

[aturon]

@nagisa So, at this point it's pretty clear that the ball was dropped on this RFC (sorry!), and of course several of the changes it proposes would be breaking at this point.

While I would still like to have some RFC'ed conventions around builders, I think at this point we need to declare bankruptcy on this RFC, take a look at what's happened in external crates (and things like net2), and start again with a fresh RFC.

If you're up for that, great! If not, I understand, and the libs team will try to make something happen here.

As per the libs team meeting, I'm going to close this RFC. Thanks for the proposal!