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

Fix LexicographicalAnnotationAttributeListing string sorting #281

Merged
merged 3 commits into from
Oct 7, 2022

Conversation

Ptijohn
Copy link
Contributor

@Ptijohn Ptijohn commented Oct 6, 2022

Suggested commit message:

Fix sorting of annotation nodes (#281)

By ignoring leading and trailing escaped double quotes.

@Ptijohn Ptijohn requested review from rickie and Stephan202 October 6, 2022 13:32
*/
return ImmutableList.copyOf(SourceCode.treeToString(node, state).split("=", -1));
return ImmutableList.copyOf(
SourceCode.treeToString(node, state).replaceAll("^\"|\"$", "").split("=", -1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the right fix tbh.
But after loads of testing, I finally got that the escaped quotes were considered in the sorting, which we actually don't want, and we want to sort what's inside the string.
Thus filtering out the leading and trailing quotes (if exists) from there.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for tracking down the source of the odd sorting! It looks like we should use ASTHelpers#constValue here; I'll push a proposal :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't know about this one :D
Thanks @Stephan202 for pushing those extras!

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Nice!

Alternative suggested commit message:

Fix `LexicographicalAnnotationAttributeListing` string sorting (#281)

In particular, the empty string is now ordered first.

*/
return ImmutableList.copyOf(SourceCode.treeToString(node, state).split("=", -1));
return ImmutableList.copyOf(
SourceCode.treeToString(node, state).replaceAll("^\"|\"$", "").split("=", -1));
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for tracking down the source of the odd sorting! It looks like we should use ASTHelpers#constValue here; I'll push a proposal :)

@Stephan202
Copy link
Member

Had a shower thought; added one more commit.

@Ptijohn Ptijohn added this to the 0.4.0 milestone Oct 7, 2022
@Ptijohn Ptijohn added the bug fix label Oct 7, 2022
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @Ptijohn ! It's nice to have this fixed after quite a while 😄.

@rickie rickie changed the title Fix sorting of annotation nodes Fix LexicographicalAnnotationAttributeListing string sorting Oct 7, 2022
@rickie rickie merged commit 5783610 into master Oct 7, 2022
@rickie rickie deleted the bdiederichs/PSM-1465 branch October 7, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants