Skip to content

Pull #13505: Restore usage of TreeSet#13505

Merged
romani merged 1 commit intocheckstyle:masterfrom
stoyanK7:copypasta
Aug 4, 2023
Merged

Pull #13505: Restore usage of TreeSet#13505
romani merged 1 commit intocheckstyle:masterfrom
stoyanK7:copypasta

Conversation

@stoyanK7
Copy link
Copy Markdown
Contributor

@stoyanK7 stoyanK7 commented Aug 3, 2023

As suggested in #13423 (comment)

please restore copy pasta and add a comment about why we use treeset

In #13423 (comment) there is a thread about a chunk of code that is a direct copy of those two:

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/CheckUtil.java

Lines 211 to 213 in ec5cd7e

public static Set<Field> getCheckMessages(Class<?> module, boolean deepScan)
throws ClassNotFoundException {
final Set<Field> checkstyleMessages = new HashSet<>();

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java

Lines 1381 to 1389 in 0c2ab53

final Set<Field> fields = CheckUtil.getCheckMessages(clss, true);
final Set<String> list = new TreeSet<>();
for (Field field : fields) {
// below is required for package/private classes
field.trySetAccessible();
list.add(field.get(null).toString());
}

There was some uncertainty as to why TreeSet is used, but @rnveach explained that in #13423 (comment)

You can't use a collection that is based on hashcode. It is JVM specific and different runs can generate different hashes and orders. We can't use insertion order here because reflection is not guaranteed an order and we want to ensure our documentation is consistent in order. It will be bad if one run says A,B,C and then another run says C,A,B. This is why we originally used TreeSet, to say order is alphabetic.
I recommend keeping this a copy/paste and just create new issues to fix anything.

This PR restores the check message keys extraction process to use TreeSet.

@stoyanK7 stoyanK7 mentioned this pull request Aug 3, 2023
Merged
@stoyanK7
Copy link
Copy Markdown
Contributor Author

stoyanK7 commented Aug 3, 2023

I will open new issue as suggested in #13423 (comment) after this is merged.

I recommend keeping this a copy/paste and just create new issues to fix anything.

But isn't the current way of having a list and sorting it good enough?

@rnveach rnveach assigned romani and unassigned rnveach Aug 3, 2023
romani
romani approved these changes Aug 4, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge

@romani romani merged commit f74293a into checkstyle:master Aug 4, 2023
@github-actions github-actions bot added this to the 10.12.3 milestone Aug 4, 2023
@stoyanK7 stoyanK7 deleted the copypasta branch August 4, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romani romani romani approved these changes

Assignees

@romani romani

Labels

miscellaneous

Projects

No open projects
Xdoc generation
Status: Done

Milestone

10.12.3

Development

Successfully merging this pull request may close these issues.

3 participants

@stoyanK7 @romani @rnveach