-
Notifications
You must be signed in to change notification settings - Fork 15
Sort set in Authorizations.toString() and add immutability test #65
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
base: main
Are you sure you want to change the base?
Conversation
keith-turner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some comments on #58 about the general direction of this. IMO we should do the following.
- Sort in toString for nicer output
- Keep Set instead of SortedSet in the impl and API. Also keep using Set.copyOf.
- Add a unit test to ensure that what asSet() returns is immutable. Try to add something and verify an exception is thrown and the source did not change.
Addressed these points in 6aee3e1. |
| @Override | ||
| public String toString() { | ||
| return authorizations.toString(); | ||
| return new TreeSet<>(authorizations).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what the penalty is here when the user doesn't care about sorting? Consider the example:
LOG.info("Authorizations: {}", authorizations);
I wonder if we should leave toString() the way that it was, and add a new method with this implementation called toSortedString or something. i'm usually on the fence regarding changes like this because we are baking in a performance penalty 100% of the time for something that we think is a good idea. I'd rather let the user choose if they want that penalty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that. Although in that case do you think its worth even creating a new method like toSortedString or should we just let the user use toSet and sort and create the string on their own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with letting users do it on their own. We should get the issue originator to weigh in on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctubbsii do you have any input on this as the creator of the issue associated with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now that Authorizations.of() takes a set (that was done in #68), we could just use the set that the user passed in, preserving whatever order it already had. If it's an immutable set already, we can just keep a reference to the same object instead of doing an unnecessary copy. If it's not, we can preserve the order when we do the copy (maybe using LinkedHashSet.addAll() wrapped with Collections.unmodifiableSet?). The only issue with this approach is that asSet() won't return something that has the SortedSet interface if it originally was a mutable sorted set, like TreeSet, but that might be an acceptable compromise).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctubbsii - are you saying we can close this and the associated issue?
Fixes #58