Consider adding builder methods that take Options

[Jayonas]

In general the new API in 0.11 works nicely, but one pain point I had was mapping Option<Foo> values (which I either calculate on the fly or have stored in a data structure) to optional builder method calls. For example, I'll have an expression which results in an Option<&str> for the origin of a Snippet, where None means that it won't have one. This worked great in the 0.10.x API because I could just assign my value to the relevant field of the annotate-snippets data structure, since it was also an Option. In the new 0.11.1 API I now either need to call Snippet.origin(...) (if my value is Some) or not call it (if my value is None), which really breaks the nice ergonomics of chaining all of the needed builder method calls.

For example:

let mut snippet = Snippet::source(buffer)
    .line_start(1)
    .fold(true)
    .annotations(...)
;
if let Some(origin) = my_optional_origin_expression {
    snippet = snippet.origin(origin);
}

Consider the potential improvement if there was a builder method that took an Option:

Snippet::source(buffer)
    .line_start(1)
    .fold(true)
    .annotations(...)
    .maybe_origin(my_optional_origin_expression)

Now the entire thing is a single expression -- which could e.g. be passed straight to another builder function -- instead of requiring a temporary mutable variable and conditional logic.

It's also interesting to consider the inconsistency with the Snippet::fold method, which doesn't have this problem. I can have a bool value (or expression) that controls whether folding should be enabled or disabled and pass it straight to fold rather than needing logic to either call fold (in the true case) or not call it (in the false) case. If fold were consistent with the other builder methods as they currently are, then it would take no parameters and always turn folding on when called. To be clear, though, I'm not advocating that you should change fold to be that way; I'm actually advocating the opposite, that you change the other builder methods (or add additional ones) for consistency with how fold currently is, i.e. allow them to be used to explicitly specify the default value.

My workaround for this was to make little extension traits and add my own maybe_* versions of relevant builder methods (so far just Snippet::maybe_origin and Annotation::maybe_label). While that's an entirely technically valid solution, I feel like there's a pretty good chance that such builder methods would end up being used often enough as to warrant their inclusion in the crate's API.

[epage]

We had discussed this when designing the current API but favored simplicity of the API as people who needed Option could still do it and on the sliding scale of complexity were a lot more likely to need to use a temporary anyways for building up the final result.

This doesn't mean it can't be re-visited but

• It would be good to continue to see how people use annotate-snippets after the upgrade to see if we are optimizing the API's design for the right cases
• It likely wouldn't happen soon-ish because it would be a breaking change (as a parallel API that takes Option feels excessive) and this doesn't seem like a big enough impediment to justify a breaking change.

[Jayonas]

I guess reasonable people can agree to disagree here. :)

My intuition is that production code would tend to have more complicated logic and expressions to calculate what to pass for an origin or a label, and in idiomatic Rust I would expect that logic to ultimately produce an Option that's None when it doesn't want to pass anything. I guess I can understand why you might assume that such logic would probably already involve using temporary (and possibly mutable) variables, and in fairness my coding style probably leans more heavily toward nested expressions than average, but for me at least 80% of the purpose and elegance of builder-style methods arises from the fact that they allow me to create and setup a whole struct inline and pass it off to something without needing to name it or disrupt the flow of a larger expression (e.g. a bunch of chained iterator calls), so having builder methods that can't handle all of the possible values feels like it defeats the whole point, almost like an anti-pattern.

I also don't think of it as a very big simplicity win to allow people to omit Some() when they call the method, whereas I think that it is a pretty big simplicity win to allow people to bypass the need for temporary mutable variables by directly passing an Option expression. My intuition would be that the current API is only actually much simpler in toy/example code where you're hard-coding a bunch of stuff for demonstration or hacking something together quickly. So it feels to me like the current API creates a small win for the already easy case at the expense of a larger win for the harder case. Given the imbalance I perceive in that trade-off (and assuming you're not going to break the builder methods that already exist), it wouldn't feel excessive to me at all to offer parallel APIs, especially since one is just a trivial wrapper around the other.

All that said, I recognize that:

1. I'm going entirely on intuition and my one anecdotal use case, while you almost certainly have actual data and experience with how people are generally using this.
2. My coding style is relatively biased toward nesting and chaining expressions.
3. As you said, people who want to use Option can still do it.

So in the end you're certainly in a better position to make decisions, and I can still get what I want with a handful of lines of trivial code, so I'm not going to put up much of a fight for changing it. I'm content to just agree to disagree.

I definitely agree with you about two things:

1. Actual usage data would be the best way to optimize the API design. I'll be curious to see how my intuition compares to reality as more people upgrade to the new API.
2. Changing these method signatures (assuming that you even wanted to) would not be big enough to justify a breaking change. I would certainly want to wait and bundle that in with any other, more important, breaking changes that might need to be made in the future.

Thanks for replying and listening.

[SamWilsn]

Have you considered using Option<T>'s implementation of From<T>? Something like:

    pub fn origin<T>(mut self, origin: T) -> Self where T: Into<Option<&'a str>> {
        self.origin = origin.into();
        self
    }

I believe that allows all of: snippet.origin("hello"), snippet.origin(Some("hello")), and snippet.origin(None). Playground example.

[epage]

One problem with impl Into<Option<T>> is that you no longer get automatic deref, so &String or &Cow<str> won't be coerced into &str.

[SamWilsn]

Good point! Guess it's more of a trade-off than I thought.