Skip to content

Issue #10086:Fix default AtclauseOrder and preserve order for documen… #17918

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Issue #10086:Fix default AtclauseOrder and preserve order for documen… #17918

wants to merge 1 commit into from
+9,997 −28

Conversation

Hariom-kr
Copy link
Contributor

#10086

  • Created @PreserveOrder annotation to mark properties that should not be auto-sorted
  • Applied @PreserveOrder to AtclauseOrderCheck.tagOrder field
  • Updated XdocsPagesTest
  • Updated Javadoc

@Hariom-kr
Copy link
Contributor Author

Github, generate website

@github-actions GitHub Actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/346fba8_20251011214231/index.html

@romani
Copy link
Member

romani commented Oct 12, 2025

Xdoc files should be changed, to reflect actual order of tags. Generated website on this PR should have new order of tags

@Hariom-kr
Copy link
Contributor Author

Hariom-kr commented Oct 12, 2025 •
edited
Loading

Hi @romani , mvn clean verify, is regenerating the meta XML file (src/main/resources/.../AtclauseOrderCheck.xml) back to the old alphabetical order. Should I update the meta generator?

@romani
Copy link
Member

romani commented Oct 12, 2025

Yes , whole reason of issue is to make html page to show actual value.
We need a bit more changes, we are close to final fix, but need few more steps

@Hariom-kr
Copy link
Contributor Author

Okay thanks, I am pushing the changes.

@romani
Copy link
Member

romani commented Oct 14, 2025

GitHub, generate website

@github-actions GitHub Actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/2a0b348_20251014050324/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/2a0b348_20251014050324/checks/javadoc/atclauseorder.html#Properties

@Hariom-kr
Copy link
Contributor Author

The build is success, updating it

@Hariom-kr
Copy link
Contributor Author

Github, generate website

@github-actions GitHub Actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/b99c00a_20251014063926/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/b99c00a_20251014063926/checks/javadoc/atclauseorder.html#Properties

@Hariom-kr
Copy link
Contributor Author

Hariom-kr commented Oct 14, 2025 •
edited
Loading

Hi @romani , my local build passed with mvn clean verify, but CI is failing can u guide me on this?

@romani
Copy link
Member

romani commented Oct 15, 2025 •
edited
Loading

Please read and watch videos at Starting_Development. Last video is about how to make CI green.
Please make CI green.

Checker, needs patches to be applied : https://github.com/checkstyle/checkstyle/actions/runs/18525806318/job/52796243069?pr=17918#step:7:11

fix: https://app.circleci.com/pipelines/github/checkstyle/checkstyle/37225/workflows/613e76ce-482e-4848-af67-958d5666480b/jobs/1101627/artifacts

@Hariom-kr
Copy link
Contributor Author

Thanks for clarification , I understood How to fix this

@Hariom-kr Hariom-kr marked this pull request as draft October 15, 2025 19:51
romani
romani requested changes Oct 16, 2025
View reviewed changes
Copy link
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.

please re-consider all your updates in import order.

config/import-control-test.xml
</subpackage>

<subpackage name="internal">
<allow class="com.puppycrawl.tools.checkstyle.internal.annotation.PreserveOrder"/>
Copy link
Member

Choose a reason for hiding this comment

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

is this required?
looks like we allow impot from same package

config/import-control.xml

<subpackage name="internal">
<subpackage name="annotation">
<allow pkg="java.lang.annotation"/>
Copy link
Member

Choose a reason for hiding this comment

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

"java.lang.*" supposed to be completely allowed, as it is jdk classes and always available.
please recheck if this is allow actually required, if yes, lest allow all sub packages of java.lang.

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AtclauseOrderCheck.java
* Default order of atclauses.
*/
@PreserveOrder
private static final String[] DEFAULT_ORDER = {
Copy link
Member

Choose a reason for hiding this comment

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

this might not be required as you put it on private List<String> tagOrder

@Hariom-kr
Copy link
Contributor Author

Thank you for the review! Apologies, I'm still learning.. Fixing it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romani romani romani requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Hariom-kr @romani