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

Migrate from typed-builder to bon #196

Merged
merged 18 commits into from
Sep 11, 2024
Merged

Migrate from typed-builder to bon #196

merged 18 commits into from
Sep 11, 2024

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Aug 29, 2024

No description provided.

Copy link
Collaborator

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

Side note: I also added your PR as a remote, and it's a bit annoying when its branch name is the same as in the base repo.

For me its also annoying when I want to create a second PR while another one is already open, so I always name them to prevent name conflicts.

And… @ayrat555 and @Veetaha: why still master, not main? 😇

Cargo.toml Outdated Show resolved Hide resolved
examples/async_reply_to_message_updates.rs Outdated Show resolved Hide resolved
src/api/async_telegram_api_impl.rs Show resolved Hide resolved
src/api_params.rs Outdated Show resolved Hide resolved
pub struct SetWebhookParams {
#[builder(setter(into))]
#[builder(into)]
pub url: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR: maybe use url::Url here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep this change for a separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my intention. This is only a comment to spark the idea, mainly for @ayrat555

src/api_params.rs Outdated Show resolved Hide resolved
@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 29, 2024

why still master, not main? 😇

Protecting the long-lived git tradition 💪

@Veetaha

This comment was marked as outdated.

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 31, 2024

Success! Indeed the trait abstractions in bon were causing the difficulty for the compiler. I managed to bring the type checking perf. of the code generated by bon to the level of typed-builder 🎉. This is currently in a quickly-edited version of bon with lots of junk that I'll refactor tomorrow and make a patch release of bon, but it works:

Here are the new compilation timings with bon:
image
Same measurements but with typed-builder:
image

There is now a 0.7s difference between the from-scratch-compilation perf of bon and typed-builder, which is much more acceptable (given that bon depends on darling additionally and compilation of bon-macros takes ~1 second, while typed-builder-macro takes 0.5 seconds to compile).

@Veetaha

This comment was marked as outdated.

@Veetaha Veetaha marked this pull request as ready for review September 1, 2024 17:32
@ayrat555

This comment was marked as outdated.

@Veetaha

This comment was marked as outdated.

@EdJoPaTo
Copy link
Collaborator

EdJoPaTo commented Sep 5, 2024

I think we should wait a bit for bon to settle. Changes will quickly create a bunch of changes in frankenstein due to its quite heavy builder usage.

I really like its benefits over typed-builder, but I would like to prevent multiple refactorings while bon is still maturing.

Also, there might be ways to improve the usage of bon even further by condensing all into conditions like on String into a single place which would reduce the human mistake rate of it?

@Veetaha

This comment was marked as off-topic.

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 8, 2024

Hey @ayrat555, @EdJoPaTo I've updated bon to the version 2.2.0 and the diff became much more negative (there is no outstanding #[builder] on structs now, the previous derive(Builder) now works).

I propose to merge this PR. See my comment #196 (comment) about the improvements suggested by @EdJoPaTo I propose to do that in a separate PR to keep the diff low and avoid future merge conflicts.

@EdJoPaTo

This comment was marked as off-topic.

@Veetaha

This comment was marked as off-topic.

@Veetaha

This comment was marked as off-topic.

@EdJoPaTo

This comment was marked as off-topic.

Copy link
Collaborator

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

Jut a nitpick, otherwise I think it's a good to merge!

This is definitely a breaking change with the changes of into but I think its worth it.

Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: EdJoPaTo <github@edjopato.de>
@Veetaha

This comment was marked as off-topic.

@ayrat555 ayrat555 merged commit 2dc02ab into ayrat555:master Sep 11, 2024
28 checks passed
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