Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add builder derive and non_exhaustive for option structs #292

Merged
merged 1 commit into from
Jun 17, 2023

Conversation

YJDoc2
Copy link
Contributor

@YJDoc2 YJDoc2 commented Mar 29, 2023

This implements the suggestions from #174 (comment)

This adds builder for each of the option structs and a non_exhaustive annotations on all of them.

It goes without saying that this is a breaking change due to the non_exhaustive.

@YJDoc2
Copy link
Contributor Author

YJDoc2 commented Mar 29, 2023

@kivikakk , I have added the builders, however the usage seems a bit too verbose to me.
Do you think flattening all the options into just ComrakOptions might be a better idea? That means we would not have separate parse,render and extension options.

@YJDoc2 YJDoc2 mentioned this pull request Mar 29, 2023
@kivikakk
Copy link
Owner

Hmmm, yep, I totally get what you mean -- it does seem pretty verbose. I think it does make sense to remove the distinction between the three different structs when using builders; that was essentially to make it easier to provide a smaller set of options, which builders help with.

That said, I'm a bit worried that it'll be less obvious what distinct kinds of options are available. This definitely ties into #291 in a big way, so I'm not exactly sure yet which direction to go.

Thanks so much for this -- I'm going to think on it now that I can really see what it looks like. I think it's close but not quite it.

@kivikakk
Copy link
Owner

Thanks for your work here, I really appreciate it! I've rebased it after this came up again in #303, #305 and #321 (!). One thing I left out from that (and this) PR is the root options struct. I'm still not convinced that we shouldn't just merge all the options into one big struct, but it still seems too ugly -_-

@kivikakk kivikakk merged commit a80bb45 into kivikakk:main Jun 17, 2023
@YJDoc2 YJDoc2 deleted the builder-v2 branch June 17, 2023 06:10
@YJDoc2
Copy link
Contributor Author

YJDoc2 commented Jun 17, 2023

Glad to help!

@Turbo87
Copy link
Contributor

Turbo87 commented Oct 3, 2023

error[E0433]: failed to resolve: could not find `OptionsBuilder` in `comrak`
  --> crates_io_markdown/lib.rs:64:17
   |
64 |         comrak::OptionsBuilder::default();
   |                 ^^^^^^^^^^^^^^ could not find `OptionsBuilder` in `comrak`

maybe I'm using it wrong, but it looks like those builder structs are not pub. maybe introduce a fn builder(Self) on the options structs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants