refactor(ibc-testkit): use bon instead of typed-builder
#1326
Conversation
|
@rnbguy, I think your review is required, since you are the author of the issue |
| pub fn init<H>( | ||
| context: &TestContext<H>, | ||
|
|
||
| #[builder(default, into)] // |
There was a problem hiding this comment.
Note that rustfmt does a bad job of formatting attributes on function arguments due to rust-lang/rustfmt#6276. Throughout the PR I tried to avoid using them (e.g. use Option type instead of adding #[builder(default)]), but here I used a workaround with an empty line comment to nudge rustfmt into placing the attribute on a separate line before the function parameter (which reads better IMO).
I described this problem in the docs about limitations.
There was a problem hiding this comment.
hey. Thanks for mentioning this. We will try it out at our end and decide. 🙌
There was a problem hiding this comment.
I took a look at the examples from your doc and tried on our codebase. For now, I am ok with having #[builder(default = ...)] arg: T in the same line 🙂
There was a problem hiding this comment.
Sure, I'll remove the workarounds
|
hey @Veetaha ! 👋 Thanks so much for this PR. 🙏 Since this touches many parts of the codebase, please allow us a few weeks to review this properly. 🙂 |
| #[bon] | ||
| impl LightClientBuilder { | ||
| #[builder(finish_fn = build)] | ||
| pub fn init<H>( | ||
| context: &TestContext<H>, | ||
|
|
||
| #[builder(default, into)] // | ||
| consensus_heights: Vec<Height>, | ||
|
|
||
| #[builder(default)] // | ||
| params: H::LightClientParams, | ||
| ) -> LightClientState<H> | ||
| where | ||
| H: TestHost, | ||
| HostClientState<H>: ClientStateValidation<DefaultIbcStore>, | ||
| { |
There was a problem hiding this comment.
hey. can you explain why you still needed a struct here ? not a dummy_light_client() method ?
There was a problem hiding this comment.
I think, I just hesitated here a bit, since it seemed to me like it wasn't just a dummy fixture. Now that you pointed this out, I'll change it to dummy_light_client().
There was a problem hiding this comment.
Btw. I made context here to be the parameter of the starting function, so the syntax is:
dummy_light_client(context) // context is passed to the starting function
.consensus_heights(...) // optional
.params(...) // optional
.build()There was a problem hiding this comment.
Ah nice ! I can think of some places, we could do this. But I will take care of it. 🙌
Also, build is the default name for finish function, right ? In that case, we can remove the (finish_fn = build).
There was a problem hiding this comment.
build is the default name for finish function only if you derive the builder for a struct with derive(bon::Builder) or if you derive the builder from the associated method named new. In these contexts, it's obvious that the builder is used for "building on object".
For other functions and methods that can execute any other arbitrary logic the default finishing method name is call(). bon can't assume what the logic of the general-purpose functions/method does. In some cases like in ballistics_rs crate the functions do some pure calculations and return primitive values, in other cases people use bon on associated methods of async API clients, client.method().param().call().await. In all of these cases the build() name of the method doesn't really fit, so a more generic name was chosen.
There was a problem hiding this comment.
ah. fair enough. I am ok with using call instead of build 👍
There was a problem hiding this comment.
Sure, I guess all things are resolved, and this is ready to merge then
There was a problem hiding this comment.
There are some huge PRs due this week. After them, I will resolve any conflicts in your PR and merge it. Thanks, @Veetaha, for your contribution ! 🤝
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1326 +/- ##
==========================================
+ Coverage 66.76% 66.83% +0.07%
==========================================
Files 225 225
Lines 22614 22633 +19
==========================================
+ Hits 15098 15127 +29
+ Misses 7516 7506 -10 ☔ View full report in Codecov by Sentry. |
Closes: #1289
Description
Hi, I'm the maintainer of the
boncrate. I found your issue #1289 and decided to work on it. While working on it I also fixed a couple of bugs inboncaused by edge cases in the handling of generics in functions return types and added a workaround to the bon's code generation for the rustc bug (rust-lang/rust#87682 (comment)). The new2.0.1patch release contains the necessary fixes, so now I can submit this PR with confidence that there are no more bugs inbontriggered by the usage patterns in your repo.Note that I know almost nothing about blockchain 🐱, and this PR is basically just a mechanical migration from
TypedBuildersyntax tobonsyntax.PR author checklist:
unclog(this internal change doesn't need a changelog entry).docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.