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

Using derive_builder to avoid repetition with the builder pattern #109

Closed
marioortizmanero opened this issue Aug 13, 2020 · 0 comments · Fixed by #129
Closed

Using derive_builder to avoid repetition with the builder pattern #109

marioortizmanero opened this issue Aug 13, 2020 · 0 comments · Fixed by #129
Assignees
Labels
enhancement New feature or request

Comments

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 13, 2020

When I was reading the code in src/oauth.rs and src/client.rs I couldn't help but notice all the boilerplate and repetition needed to implement builder patterns for some of the structs declared.

I think this would be a great opportunity to use the derive_builder crate in order to have the codebase more maintainable, easier to read, and consistent. Read more about the features it provides here.

The downside is possibly longer compilation times, but after improving this part in the latest commits I wouldn't think it's much of a problem. And I would like to compare the compilation times before and after the changes before this is merged into master to see how much of a difference there actually is.

Furthermore, using From<String> instead of &str for the builder parameters like this one would avoid an extra allocation for whenever the user passes a String, since the to_owned() can be omitted, and it will be directly moved into the struct. If the user passes a &str it will just be cloned. This is also supported by derive_builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant