Make the two wikilinks extensions mutually exclusive (and other assorted improvements)

[SamWilsn]

Noticed some weirdness with the commonmark formatter and having both extensions enabled ([[title|url|title]] kind of thing.)

[digitalmoksha]

In general you're right, they should be mutually exclusive. But it seems reasonable that the behavior is undefined if you set both of them 😄

Also, doesn't this change break backward compatibility? 🤔

[SamWilsn]

In general you're right, they should be mutually exclusive. But it seems reasonable that the behavior is undefined if you set both of them 😄

I see you're a fan of nasal demons.

Also, doesn't this change break backward compatibility? 🤔

Yes, unfortunately. I started out by deprecating the booleans in the builder and writing custom setters to make them behave properly, but I'm not particularly experienced with the bon crate.

[Veetaha]

There is this guide about adding custom setters, although I pointed right in the middle of it.

[Veetaha]

However do we need this backwards-compat crutch at all though? @digitalmoksha WDYT, maybe a breaking change is fine if this API is not widely used?

[digitalmoksha]

TIL about nasal demons!

@Veetaha @SamWilsn well, I don't know how many people are using it. I know I am. And commonmarker uses it, and I did see it integrated into at least one other package that uses comrak, but I can't find it right now.

I modeled this off of Pandoc's syntax. And it mentions in the release notes

Pandoc will use wikilinks_title_after_pipe if both extensions are enabled.

I think I would come down on the side of mimicking Pandoc's behavior, without a breaking change.

[SamWilsn]

You don't find that behaviour somewhat unexpected? IMO, if a weird state can be made impossible, it should be.

[digitalmoksha]

Not really - it does something sensible. It's a forgiving interface. Same way in many systems if you specify an option multiple times, the last one gets used, as opposed to generating a hard error. (I checked, comrak generates an error if the same option is specified multiple times on the command line).

However I think it would be ok to generate an error for this. But what does that look like when using the API, where every entry point returns a String? I don't think panicking would be the right behavior.

[Veetaha]

I modeled this off of Pandoc's syntax. And it mentions in the release notes

Makes sense. However, pandoc is CLI, so it follows the CLI conventional design there. I would agree if comrak's explicit goal was to follow pandoc's API 1-to-1, and be as low-level as that. However, I think that's not the case, and for comrak's higher-level Rust API an enum could be more idiomatic in this case.

I believe the path of least disruption would indeed be to keep the old boolean setters for backwards compat (they just set the internal enum value). Then, mark them with #[deprecated = "use enum setter instead"].

Note that this will still be a breaking change for people that have this pattern in their code:

        .wikilinks_title_after_pipe(true)
        .wikilinks_title_before_pipe(true)

I.e. if they call both setters. This is because bon disallows setting the same value twice (the code just won't compile). It can however, be worked around with overwritable. But maybe such kind breakage is actually fine since it probably points to an actual bug?

[digitalmoksha]

wrong code posted before. what about

    let mut comrak_options = comrak::ComrakOptions::default();

    comrak_options.extension.wikilinks_title_after_pipe = true;
    comrak_options.extension.wikilinks_title_before_pipe = true;

edit: the values are typically provided by another set of options

[SamWilsn]

Having to preserve the pub fields in the struct is why I abandoned backwards compatibility.