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 bon for an infallible and compile-time-checked builder #466

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Sep 8, 2024

Hi! This PR replaces the usage of derive_builder with bon. What does it give us?

  • The build() method no longer returns a Result<T>, it returns T instead. So there are no potential panics and no need for error handling in runtime. For example:
    image
  • The builder generated by bon validates that the same field isn't set twice. For example, with bon I detected this mistake in tests where math_dollars and math_code were set twice, because bon generated a compile error

    comrak/src/tests/api.rs

    Lines 62 to 67 in 1a33c63

    extension.math_dollars(false);
    extension.math_code(false);
    extension.front_matter_delimiter(None);
    extension.multiline_block_quotes(false);
    extension.math_dollars(false);
    extension.math_code(false);
  • bon generates a T::builder() method, which removes the need for importing the TBuilder type and using TBuilder::default() syntax to create the builder.
  • If you ever need some more complex logic of building, or you want to avoid create a separate "parameters" struct, you can always generate a builder from a function or from a method with bon. Builders generated this way use the same API conventions and it is even possible to switch between struct/function syntax and keep your crate API the same (see e.g. switching from #[derive(Builder)] to a #[builder] on the method named "new")

I understand this is a breaking change for this crate, but I think improvements are worth it.

.strikethrough(exts.contains(&Extension::Strikethrough) || cli.gfm)
.tagfilter(exts.contains(&Extension::Tagfilter) || cli.gfm)
.table(exts.contains(&Extension::Table) || cli.gfm)
.autolink(exts.contains(&Extension::Autolink) || cli.gfm)
.tasklist(exts.contains(&Extension::Tasklist) || cli.gfm)
.superscript(exts.contains(&Extension::Superscript))
.header_ids(cli.header_ids)
.maybe_header_ids(cli.header_ids)
Copy link
Contributor Author

@Veetaha Veetaha Sep 8, 2024

Choose a reason for hiding this comment

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

For optional fields (or fields with #[builder(default)]) bon generates two setters:

  • <field>(value: T)
  • maybe_<field>(value: Option<T>)

This is desingned this way so that changing a required field to optional is compatible, and it also simplifies the syntax for optional members by removing the need to wrap the value with Some() using the <field> setter like here, for example:
image

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 10, 2024

UPD: bon 2.2.1 added support for MSRV 1.59.0

@kivikakk
Copy link
Owner

Thanks so much for all the effort you put into this! This is amazing, it looks really nice, thank you!

I'll rebase it and get it passing CI. :)

@kivikakk kivikakk merged commit b27a3dd into kivikakk:main Oct 22, 2024
20 checks passed
@kivikakk
Copy link
Owner

Thank you again! 🤍

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