Future-proof Options before 1.0

[Ekleog]

Hey! I was having a look at markdown, and noticed that the latest 1.0 alpha has Options that are structs.

I'm just coming to say, if you make it into a builder then it'll allow you to add options with minor releases, whereas with a struct like right now any option addition would require a full major release.

(This is also something I'd probably need: I'll probably soon-ish want to compile inline-only markdown to "markdown-html" (html that retains the markdown tags), and if I want any chance to upstream it I'll need the possibility of adding both parse and compile option. That said I'm not even using this crate yet, so this may totally never happen)

For the same reason you may want to make the Node enum non-exhaustive, though I'm less sure about this one. But the more stuff you can make non-exhaustive / made out of builder patterns with private members without hindering user experience, the less likely you'll need major updates in the future :sweat_smile:

[wooorm]

As I understand, #[non_exhaustive] can solve all these issues, right? Forcing folks to spread the defaults in? And a builder pattern is another, different, way to solve it, for options?

Re “inline only”, I’m not sure I think that’s a good feature to have. Depends…

[Ekleog]

For options (structs)

So I've seen #[non_exhaustive] used mostly on enums up to now. I guess non-exhaustive structs would require code like:

let mut options: Options = Default::default();
options.foo = true;
options.bar = true;
do_something(options);

which is less pleasant than (builder-pattern equivalent):

do_something(Options::builder()
    .foo(true)
    .bar(true)
    .build());

The builder pattern is basically adding methods to Options to enable the second syntax, whereas afaiu non-exhaustive structs can currently not be constructed even by completing them with ..Default::default().

(This being said, both options do avoid the issue that adding one field is a breaking change; it's just that one is easier to handle for the library user)

Note also that there are crates like https://crates.io/crates/derive_builder to reduce the boilerplate of writing builder patterns :)

For enums

Now, I also mentioned making the Node enum non-exhaustive; this is mostly to avoid that people do a match on it: without it, adding one variant to Node would be a breaking change.

(As for the inline-only, I'm not opening this issue to discuss it yet, we can chat about it if/when I actually get to it; it was just to showcase one possible example of a reason why being able to add options could be useful :))

[wooorm]

All the examples in the docs use the spread syntax, e.g.: https://docs.rs/markdown/1.0.0-alpha.3/markdown/fn.to_html_with_options.html#examples. So the case you show above, with spreading, would be:

let options = Options {
    foo: true,
    bar: true,
    ..Default::default()
}

I strongly prefer this because it does not force users to learn a new API. And it already prevents new fields from becoming a problem

[wooorm]

Ah, so adding #[non_exhaustive] on a struct means you can’t use ..Default::default() anymore? That seems like a bad design.

[Ekleog]

Ah, so adding #[non_exhaustive] on a struct means you can’t use ..Default::default() anymore? That seems like a bad design.

Right, that's my understanding of this sentence:

Non-exhaustive variants (struct or enum variant) cannot be constructed with a StructExpression (including with functional update syntax).

from here at least.

I don't know why it is this way, but I guess the fact that the builder pattern is so prominent in the crate ecosystem means that it's just that non-exhaustive structs never received much love, as builder patterns allow for a more flexible mapping between options-exposed-to-the-user and the internal datastructure used by the library

[wooorm]

I think https://github.com/rust-lang/rfcs/pull/2008#issuecomment-304142837 is a reference that the behavior I’m looking for here could be allowed, but it never was.

I don’t worry about Constructs, because it has so many fields, I don’t think anyone will write that out exhaustively. ParseOptions and CompileOptions are probably also fine, as they have enough fields. But Options likely has too few fields for people to spread defaults in. On the other hand, it is unlikely that new fields will be added to Options.

[…] as builder patterns allow for a more flexible mapping between options-exposed-to-the-user and the internal datastructure used by the library

I don’t really see how a struct isn’t flexible enough. You can define the fields you want, spread different defaults (commonmark/gfm/etc) in, mutate it if you want to.

My experience, particularly from the JavaScript world, is that I strongly prefer no API (plain objects and arrays, raw access to data) over having to learn an API.

[…] it's just that non-exhaustive structs never received much love […]

Yeah… :'(

[andrewbaxter]

I'm more of a user, but working on my own library for something else and I probably have the same issue (struct api, prefer simple objects...)

As long as users do MyStruct { fields, ..Default::default() } there shouldn't be an issue, right? It looks like ..Default::default() is allowed even when all fields are specified, so with a note in the docs suggesting spread for compatibility it should be sufficient I feel like...

[Ekleog]

I understand your positions, and if I were writing javascript I'd follow the javascript conventions that are along the lines of using untyped objects to make the API ; but in rust conventions saying that users must use ..Default::default() is normally not considered as a good thing.

Basically, in Rust adding a field to an exhaustive struct should be considered the same as changing a rarely-used field name in a JavaScript struct. Which would go against semver guarantees, and make some debian people even more unhappy about rust being used in software for their stability-focused os :sweat_smile:

Also, whether you are using pub fields or a builder pattern, cargo doc should generate documentation that is just as good, though cargo doc is also optimized for readability of public functions rather than public struct fields.

Now I personally don't care that much as I'm expecting to -use Default::default(), so unless you have questions for me this will be my last message here, but I can only encourage you to go ask around the rust community about how people feel on configuring a library using an exhaustive struct with pub fields, vs. a non-exhaustive struct with pub fields, vs. a builder :)

[wooorm]

I’m open to a PR. Do you want to work on this Léo?

[Ekleog]

I just did what I could in #40 :)

I didn't future-proof the pub structs in mdast.rs, because it didn't appear worthwhile to me. If you plan on often adding things to these structs you probably should consider it, but with very rare changes, just making a major release upon adding fields would probably fit the bill too.

Also, please remember that it is my first time interacting with markdown-rs, so it's very possible that I missed stuff that should be made future-proof. There are also builder-helper methods that might make sense that I didn't add (eg. an allow_dangerous that'd make both allow_dangerous_html and allow_dangerous_protocol might be useful in tests seeing how often I had to fix the two together), but I didn't check whether it also made sense outside of tests so I didn't add them in.

[bnchi]

I'm working on #127 and was thinking of making the options passed to the to_markdown function also configurable using a builder I was thinking that once the user call build we would do error checks to know if the user provided a valid option instead of doing the check in the serialization step.

What do you think @wooorm ?

[wooorm]

Seems sensical indeed, to check things, when using a builder pattern? Perhaps I do not understand the exact effect your two alternatives have on the code :)

[bnchi]

What I have in my head is along the lines of this code :

struct Options {
 strong: char,
}

struct OptionsBuilder {
    strong: char,
}

impl OptionsBuilder {
    fn new() -> Self {
        Default::default()
    }

    fn strong(mut self, strong: char) -> OptionsBuilder {
        self.strong = strong;
        self
    }

    fn build(self) -> Result<Options, OptionsErrorMessage> {
        let options = Options {
            strong: self.strong,
        };
        
        // check strong
        if options.strong != '*' {
            // return an error
        }

        Ok(options)
    }
}

I hope this makes it clear

[bnchi]

In the JS implementation check_strong is being called during serialization here : https://github.com/syntax-tree/mdast-util-to-markdown/blob/main/lib/handle/strong.js#L24

using the above method I believe we can check if the user provided the valid options at the build step

[wooorm]

Oh. I would do such checks in strong probably?

In JS things are checked “dynamically”, because options can also be inserted dynamically. For Rust, while this does not happen, if a manual Options object is supported, I do think that also has to be checked. If you only check in build, then it is still possible to pass invalid options?

[bnchi]

If you only check in build, then it is still possible to pass invalid options?

The Options fields will not be pub that's the user from a different module will not be able to set Options fields without going through the builder, if none of the Options fields are pub a user can't just construct Options.

For getting the values we can use getter methods on Options

[wooorm]

Ah, hmm. Not sure about that. About only allowing “builder”. To be honest, I think it’s rather weird in Rust that you can’t use structs/objects for options.

[bnchi]

The pattern guarantee a valid state for options and encapsulation, if you've worked with the reqwest crate before you can see they do the same, you can configure a Client using the ClientBuilder setter methods, this pattern is pretty much used heavily in the Java land with getters and setters too "I guess till now" my Java skills is a bit rusty these days.

In my PR I'm going to try not use the pattern for now.

Thank you !

[wooorm]

If reqwest does it, that might be interesting. Java doesn’t do much for me ;) But I also learned, over the years, maintaining libraries for people, that it’s good to be as simple as possible. Plain objects over “instances” and the like. That’s my experience 🤷‍♂️