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

Removing the with_ prefix from builder methods #113

Closed
rfuest opened this issue Feb 3, 2024 · 2 comments
Closed

Removing the with_ prefix from builder methods #113

rfuest opened this issue Feb 3, 2024 · 2 comments

Comments

@rfuest
Copy link
Collaborator

rfuest commented Feb 3, 2024

Should we remove the with_ prefix from the builder methods? The API guidelines state that the with_ prefix should be used for constructors (https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case) and builders usually don't use and prefix for the methods (https://rust-lang.github.io/api-guidelines/type-safety.html?highlight=builder#builders-enable-construction-of-complex-values-c-builder).

The naming is confusing in the latest release where Builder::with_model is a constructor, while other methods, like Builder::with_orientation, are used to set a parameters. In the master branch with_model was renamed into new, which makes it harder to mix up the two types of methods, but I would still prefer to remove the prefix.

@almindor
Copy link
Owner

almindor commented Feb 3, 2024

Hmm so you mean it'd look like Builder::new(<model>).orientation(Orientation::Portrait)... ?
I'm a bit on the fence. Something like Builder::new(...).orientation(Orientation::Portrait) seems a bit odd to me

@rfuest
Copy link
Collaborator Author

rfuest commented Feb 4, 2024

Yes, that's what I meant. It is the recommended way of naming builder methods and should be familiar to many users, because it is used in many places like https://doc.rust-lang.org/std/process/struct.Command.html, https://docs.rs/clap/latest/clap/struct.Arg.html or https://docs.rs/embedded-graphics/latest/embedded_graphics/text/struct.TextStyleBuilder.html.

almindor added a commit that referenced this issue Feb 15, 2024
remove `with_` prefix from Builder, fixes #113
DivineGod added a commit to DivineGod/mipidsi that referenced this issue Feb 5, 2025
Updates to the names of methods on the `Builder` merged in PR almindor#113 missed updating the TROUBLESHOOTING.md file
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

No branches or pull requests

2 participants