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]

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".

[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!