I'm writing a builder struct (the self-mutating kind, not the self-consuming kind), and some of its methods take strings that must follow certain formats (a different format for each method). The obvious implementation is to make these methods return
Result<&mut Self, Error>, with
Err(Error) returned if the string is invalid. However, I've never seen builder methods that return
Result before, and I suspect that such a method may violate best practices in some way. The alternative is to create dedicated types for each argument string format so that the type system ensures validity, but this seems like overkill; in particular, the only difference from the user's perspective would be that
builder.some_param("foo".parse()?) — more typing (with a keyboard) for more typing (with a type system).
What say you, Rustaceans? Are builder methods that return
Result<&mut Self, Error> acceptable, or should the user be required to
parse() their string arguments instead? Something else?
Using separate types would let you reuse the validation in other places, but if you never need that I agree it's probably overkill.
I think traditionally all of the validation logic that might fail is moved to the
build method that actually constructs the new type from the builder. I wouldn't say there's necessarily anything wrong with returning a
Result from a builder method though.
build() return a
Result is not an option that occurred to me. While I can see the appeal, I don't really like the idea of the error occurring at a different point than where the invalid argument was given, as either the user wouldn't know which argument was bad or else I'd have to add a parameter identifier to the error struct.
? to the end is a 1-character change so I would say it's perfectly acceptable for a builder method to return a
Result<&mut Self, Error>. That also allows you to tack on extra context (e.g.
builder.some_param("foo").context("Unable to parse some param")? if you use
Builders are meant to be convenient after all, so you'll often accept things like
impl Into<String> or
impl SomeTrait + 'static to allow users to pass in more types. I'd be perfectly fine if you had something like
fn with_url<U>(&mut self, url: U) -> Result<&mut self, Error> where
U: TryInto<Url>. That would open the door for passing in string literals (e.g. during tests or when the value is hard-coded) or a pre-parsed
Url object you already have.
I think that's overselling it a bit. It's only a one character change in a context where
? can convert to your return type. A library that goes from infallible to
Result isn't typically a one character change for the consumers, it's at least a
impl From or such.
That said, if your system is built around parsing, there's no avoiding being fallible, in which case I'd agree it's perfectly acceptable to return a
Result (and preferrable to panicking, say).
But if you go the more typing for the sake of more types route, it's often possible to provide an infallible alternative, such as an
enum to represent the parsed results. If I can code the infallible
then I don't need to code the fallible
I think both the individual steps and the final build method returning
Results is reasonable, depending on the kinds of errors.
.name(the_name).unwrap() might make sense if it can validate the name in isolation somehow. Then
.build() being fallible would be for things that can only be checked by looking at a combination of fields.