-
Notifications
You must be signed in to change notification settings - Fork 611
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
Replace derive_builder
with bon
#9712
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9712 +/- ##
==========================================
- Coverage 88.93% 88.91% -0.02%
==========================================
Files 288 288
Lines 29630 29609 -21
==========================================
- Hits 26350 26326 -24
- Misses 3280 3283 +3 ☔ View full report in Codecov by Sentry. |
@Turbo87 regarding the custom dummy checksum finishing function (method on the builder that consumes the builder and returns the result). It should be possible to add custom methods to the builder itself in the next release. It will be a 3.0 release, but breaking changes there won't touch 99% of the users since they break some rare obscure cases and rarely used apis such as The builder type state API will be stabilized (the PR for that was recently merged into master elastio/bon#145). Thank you for the feedback! |
Regarding the usage in test fixtures. In 3.0 it will be possible to annotate the type of the builder, so test fixtures could benefit from this by returning a builder with (potentially partially) filled values like so: #[derive(bon::Builder)]
#[cfg_attr(test, builder(on(_, overwritable)))]
struct User {
login: String,
name: String,
level: u32,
}
#[cfg(test)]
mod tests {
use super::*;
use user_builder::{SetLevel, SetLogin, SetName};
// Base fixture that has dummy values for all or maybe just part of the fields
fn user() -> UserBuilder<SetLevel<SetName<SetLogin>>> {
User::builder()
.level(24)
.name("Bon Bon".to_owned())
.login("@bonbon".to_owned())
}
#[test]
fn test_with_empty_name() {
// Build user with an empty name, and all other fields filled with dummy data
let user = user()
.name("".to_owned())
.build();
// ...
}
#[test]
fn test_with_zero_level_and_admin_login() {
let admin = user()
.level(0)
.login("@admin".to_owned())
.build();
// ...
}
} Although, I'm not sure if there will be big demand for this. Notice the |
pub fn features( | ||
&mut self, | ||
features: &BTreeMap<String, Vec<String>>, | ||
) -> serde_json::Result<&mut Self> { | ||
self.features = Some(serde_json::to_value(features)?); | ||
Ok(self) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there will be a new attribute in 3.0.
that will allow expressing this setter as this (apart from defining the custom method on the builder directly):
#[builder(with = |features: &BTreeMap<...>| -> serde_json::Result<_> {
serde_json::to_value(features)
})]
features: serde_json::Value,
Yes it's possible to have a fallible setter this way if you annotate the return type of the closure with a type that has Result
suffix. The lack of return type annotation implies the closure passed to with
is infallible.
awesome, thanks for the quick feedback :) |
bon
uses the type system to ensure all required fields have been set, which means that we can catch issues at compile time. The necessary attributes are also less verbose due to better defaults insidebon
.The only downside I've found is that custom builder fns like
dummy_checksum()
don't appear easy to implement at the moment, which is why I've inlined the two custom builder fns that we had. (/cc @Veetaha)