Skip to content

Conversation

@DomGarguilo
Copy link
Member

Fixes #58

Copy link
Contributor

@keith-turner keith-turner left a 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.

@DomGarguilo
Copy link
Member Author

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.

@DomGarguilo DomGarguilo changed the title Use sorted set in Authorizations Sort set in Authorizations.toString() and add immutability test Feb 22, 2024
@DomGarguilo DomGarguilo requested a review from dlmarion February 22, 2024 19:23
@Override
public String toString() {
return authorizations.toString();
return new TreeSet<>(authorizations).toString();
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Member

@ctubbsii ctubbsii Jul 18, 2024

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).

Copy link
Contributor

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?

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.

Authorizations set view could be sorted

4 participants