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

Implement builder pattern #168

Merged
merged 6 commits into from Oct 4, 2022
Merged

Implement builder pattern #168

merged 6 commits into from Oct 4, 2022

Conversation

sharifhsn
Copy link
Contributor

As discussed in my previous pull request, this pull request deprecates using the new method directly on the Printer struct, preferring the PrinterBuilder struct for the builder pattern. This is a breaking change for hexyl as a library.

@sharifhsn
Copy link
Contributor Author

Let me know if there are any bikeshed quibbles with the naming of methods and I'll be happy to change them!

@sharkdp
Copy link
Owner

sharkdp commented Oct 3, 2022

Awesome, thank you very much!

Could you please add an entry to the "unreleased" section in CHANGELOG.md to briefly describe the breaking change? That would help me with creating the next release.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@sharifhsn
Copy link
Contributor Author

One last thing: do you feel it's necessary to add documentation to the new PrinterBuilder struct? I feel like the builder functions are self-documenting, but perhaps the new function and build function should have examples for those unfamiliar with the builder pattern.

@sharkdp
Copy link
Owner

sharkdp commented Oct 4, 2022

One last thing: do you feel it's necessary to add documentation to the new PrinterBuilder struct? I feel like the builder functions are self-documenting, but perhaps the new function and build function should have examples for those unfamiliar with the builder pattern.

Thank you for your concern. Adding documentation is always great, but I wouldn't see this as a requirement for this PR. This changeset already improves hexyl as a library significantly.

There was no documentation before because - to be honest - I only support hexyl-the-public-library in a "best effort" manner.

So I suggest we merge this as is, but new PRs are always welcome of course!

@sharifhsn
Copy link
Contributor Author

That should be everything!

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit 9602667 into sharkdp:master Oct 4, 2022
@sharifhsn sharifhsn deleted the builder branch November 14, 2022 14:58
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.

None yet

2 participants