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

Mutable methods are now protected #214

Closed
wants to merge 1 commit into from

Conversation

gastaldi
Copy link
Contributor

@gastaldi gastaldi commented Apr 3, 2021

In order to preserve the simplicity of the API and keep tests easy to write, I made the methods that change the internal state in PrettyTime package-protected.

This will require a major version bump.

Fixes #211

@lincolnthree
Copy link
Member

lincolnthree commented Apr 4, 2021

Hey George, thanks for this :) I appreciate the PR.

I think the API needs to be exposed in some form, so this is an interesting idea. I guess that would work, but would require sub-classing - does it really solve the problem of immutability, or does it just push it downstream? You mentioned previously that the mutators should return new instances. I think that approach is probably more consistent with the concept of immutability.

Or did you have something else in mind? Does this actually solve the issue in Quarkus? I guess it prevents modification, so that would work. Yeah, just thinking out loud but I'm not sure making them protected is the right answer there. If anything, creating a wrapper class for Quarkus would make more sense than altering the existing API? Possibly a new builder facade?

Sorry again I've been so busy. Still trying to play catch-up with the launch.

@gastaldi
Copy link
Contributor Author

gastaldi commented Apr 4, 2021

Hey Lincoln!

I thought about introducing a Builder facade but at the same time I wanted to keep the API simple, which is what the existing constructors already provide.

A wrapper class in the Quarkus extension may make sense, but I believe this must be a core API design change.

I'll revisit the idea of exposing mutators but returning new instances, probably that is indeed a better solution

@gastaldi gastaldi closed this Sep 8, 2021
@gastaldi gastaldi deleted the immutable branch September 8, 2021 19:20
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.

Make PrettyTime immutable
2 participants