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

Builder pattern naming #112

Open
carlocorradini opened this issue Jun 9, 2024 · 2 comments · May be fixed by #113
Open

Builder pattern naming #112

carlocorradini opened this issue Jun 9, 2024 · 2 comments · May be fixed by #113

Comments

@carlocorradini
Copy link
Contributor

I have a very simple proposal for a small change to the builder pattern naming.
I had no idea that I could use a builder to customize the client until I started looking into the code.
Most implementations include a builder method that returns the builder, then a build function that generates an instance of the (built) class.
Because the builder function returns an instance of the builder and the build method returns an instance of the "buit" class, there is a distinction between the two.
To help new users understand that the client is not built directly but rather provides a builder that must subsequently be invoked with the build function in order to return an instance of the class, I suggest renaming this to builder.

@obmarg
Copy link
Owner

obmarg commented Jun 9, 2024

I don't disagree with anything you've said, but I am cautious of making a breaking change.

I've had 3 breaking releases in the past 3 or so months, and I'm not sure the benefits of this rename would outweigh the annoyance of another breakage.

Happy to keep it in mind for the future though.

@obmarg
Copy link
Owner

obmarg commented Jun 9, 2024

Actually, what I'd probably prefer to do is add a Client::builder function as you suggest, and then mark Client::build as deprecated. Then in a few releases I can remove the deprecated function.

Though I am still a little cautious of doing that - I'd prefer to avoid making a release thats only purpose is to deprecate that function. But once some other releasable changes come along I think I'd be fine including the deprecation along with them.

@carlocorradini carlocorradini linked a pull request Jun 11, 2024 that will close this issue
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 a pull request may close this issue.

2 participants