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

Use a builder pattern for public structs, for future-proofing #40

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ekleog
Copy link

@Ekleog Ekleog commented Jan 2, 2023

See discussion at #33

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

  • Why is the pub(crate) stuff needed?
  • I think the helper fns for mdx, gfm, and such, should not generate a new options struct, but instead operate on an existing one, that way, it could be used as ::default().gfm(true).mdx(true), to turn on both
  • should we still expose the structs too, and then also expose *Builder for each? Why not only expose builders, under the original name? As an example, why CompileOptionsBuilder::default().allow_dangerous_html(true).build(), instead of something like compile_options().allow_dangerous_html(true).build()
  • Can you rebase? :)

@@ -447,6 +450,7 @@ pub enum AttributeContent {

/// MDX: attribute value.
#[derive(Clone, Debug, Eq, PartialEq)]
#[non_exhaustive]
pub enum AttributeValue {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are supposed to be exhaustive?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For AttributeValue and AttributeContent, I'll trust you, as I don't even know what they're about as I never used JSX markdown ^^'

For Node, I'm pretty sure keeping it non-exhaustive would be better: it seems likely to me that some future evolution will require adding node types given the length of the enum (eg. supporting some new variant of markdown?). And someone who already filled match arms for each of the variants can probably also fill in a default match arm for any future evolution 😅

For ReferenceKind, it's kind of the same train of thought, but I'm less sure about myself.

Does my thinking make sense? If so, which ones should I turn exhaustive back?

@Ekleog
Copy link
Author

Ekleog commented Jan 3, 2023

* Why is the `pub(crate)` stuff needed?

It allows other modules inside the same crate to access the value of these properties. The option structs are not defined in the same module as the place where they're used from, so this is required to allow access to them from there.

* I think the helper fns for `mdx`, `gfm`, and such, should not generate a new options struct, but instead operate on an existing one, that way, it could be used as `::default().gfm(true).mdx(true)`, to turn on both

I see what you mean, and had the same thought, but I couldn't find a crate to easily do that without manually writing all the code. (to be precise, safe-derive-builder does do it, but has 1k downloads and the last update is from 5 yrs ago, plus exponential build times, considering the number of parameters, sounds bad).

Looking at it again, literally while writing this comment I just came across derive-setter, that could probably be exactly what you're looking for.

However, to be quite honest, fixing all the tests to use the new API definitely exhausted my will to contribute this for now, and I won't actually do the work of porting them to a new API again, as that'd be almost as long as starting from scratch again. Sorry about that!

But yes, porting this PR to derive-setter would probably be the best. I still think that this PR is already an improvement, and will make any future switch to derive-setter easier; but I'll let you judge on that.

* should we still expose the structs too, and then also expose `*Builder` for each? Why not only expose builders, under the original name? As an example, why `CompileOptionsBuilder::default().allow_dangerous_html(true).build()`, instead of something like `compile_options().allow_dangerous_html(true).build()`

This could work, but understanding the control flow when nesting three layers of options would start being hard, with rustfmt (necessarily) not being aware of the fact that compile_options/build form a pair. Also, I think even derive-setter would not be able to auto-generate that kind of code.

* Can you rebase? :)

Well, knowing the above, do you think it'd be useful that I rebase? If yes, I'll be happy to, to push this PR past the finish line, but I can't promise more

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.

2 participants