Issue: #6311 new check: OrderedProperties #6343

thomassenger · 2019-01-03T09:58:43Z

The OrderedProperties Check is almost ready.
There are the following open issues:

builds only with: mvn package -Dcheckstyle.ant.skip=true -Dcheckstyle.skip=true -DskipTests
-- I need help with failure: XdocsPagesTest.testAllCheckSections:356->validateCheckSection:451->validateParentSection:1300 config_misc.xml section 'OrderedProperties' should have matching parent expected:<[TreeWal]ker> but was:<[Chec]ker>

rnveach · 2019-01-03T11:47:46Z

@thomassenger Please work on making CI green. We usually won't review the code until CI is green. Let us know if you are having any issues and need our help.
As you work on code, please keep number of commits as 1.

rnveach

CI should be made green.

TC and travis are failing.

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

src/xdocs/checks.xml

src/xdocs/config_misc.xml

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

pom.xml

rnveach · 2019-01-07T17:56:37Z

@thomassenger Please don't mark my points as resolved. Leave that to me to do to reverify everything you have done.
To points you have done, just reply "Done".

pom.xml

thomassenger · 2019-01-13T13:37:38Z

Thank you for your work.
Next week I am busy and I have not time to continue.
Line numbers need more investigation from my side. I will try to solve it later.
Regards,
Thomas

romani · 2019-01-27T02:10:08Z

Please reply as done to each raised items to let know that you are ready for review again.
CI should be green, please work to satisfy automatic review first.

rnveach · 2019-01-27T02:15:39Z

When CI is green I will re-review unless you need help. You may want to squash all commits to 1 and rebase off the latest master as there are conflicts.

rnveach · 2019-02-01T02:19:33Z

@thomassenger Please squash the commits into 1, don't leave a huge pile of commits in the PR. The branch is still unmergable and CI is still failing.

thomassenger · 2019-02-05T16:04:49Z

Hello, I squash all commits into one. I cannot see your last requested change anymore.
The last change I did was the line numbering.
The build is still in progress. Lets see what the outcome is. Locally was the build fine.
Regards,
Thomas

rnveach · 2019-02-05T16:43:19Z

I cannot see your last requested change anymore.

Click the "Show resolved" and "Load more..." to show the requested changes that are now being hidden.

rnveach

I will start looking into this to help.

Please rebase on latest master and fix the following errors from TeamCity.

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

src/test/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheckTest.java

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

rnveach

changes for pitest.

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

romani · 2019-02-22T06:10:50Z

@thomassenger , CI is green, I hope you are not exhausted, to pass review by humans

thomassenger · 2019-02-22T07:20:35Z

Yes I see. Unbelievable. ;-)

thomassenger · 2019-04-29T08:54:17Z

Hello Romani,
Thank you. Yes I agree. I will remove the newline functionality and add this as a limitation to the documentation.
Regards,
Thomas

thomassenger · 2019-04-30T08:40:04Z

I think all changes are done. No actions on my side.
Regards,
Thomas

romani

I do not see link to regression report, please share it one more time.
regression report we do by our tool https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation to make sure that no exception happening on big real projects and violations are valid.

items to improve:

src/xdocs/config_misc.xml

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

src/test/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheckTest.java

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

thomassenger · 2019-05-07T06:41:02Z

the requested report
groovy diff.groovy --localGitRepo ~/git/github/checkstyle/ --baseBranch upstream/master --patchBranch master --config my_check.xml --listOfProjects projects-to-test-on.properties

reports.zip

romani

reports.zip

please share report at github.io (see in instruction in README of tool) , not only I need to review it, report is designed to ease reference of case.

items to improve:

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

.ci/jsoref-spellchecker/whitelist.words

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java

src/xdocs/config_misc.xml

romani · 2019-05-17T05:23:00Z

@thomassenger , ping

thomassenger · 2019-05-18T13:16:43Z

Hello Romany, I was busy last week and continue soon.
Regards

thomassenger · 2019-05-21T14:32:30Z

To publish diff.groovy report seem to be difficult. The master branch should be orphaned. But I have already an master branch. I could use a different branch name.
I made a different repo: https://thomassenger.github.io/testrepo/
The changed the docu as requested.

romani · 2019-05-22T13:10:39Z

code is ok to merge.
Please squash commits in one, we are very close to PR merge.

We need regression report to make sure Check is not failing on real code.
provided link has no violations in report, and it is not clear on what project you did run.
Please provide details of your difficulties with our tool execution, we will help.

thomassenger:master

yes, just make git checkout -b issue-6311 on master it will create new branch on your local.
and do git checkout master and git reset HEAD~2 --hard (this will reset your master to state of our master)
run tool.

Attention: you can make new PR from your new branch to make all further steps easier. this PR can be closed in favor of new PR.

romani · 2019-05-22T13:16:20Z

Wercker is failing:
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module MissingJavadocMethod - Unable to instantiate 'MissingJavadocMethod' class, it is also not possible to instantiate it as .......... Please recheck that class name is specified as canonical name or read how to configure short name usage https://checkstyle.org/config.html#Packages. Please also recheck that provided ClassLoader to Checker is configured correctly. at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild (TreeWalker.java:136)

but our master build is not failing, please rebase your code to most recent our master code to fix issue.

Teamcity failure can be ignored, smth wrong on Teamcity side.

romani · 2019-05-22T13:49:51Z

@thomassenger , we will have a release at weekend, it would be awesome to finish this PR.

thomassenger · 2019-05-23T20:18:46Z

We need regression report to make sure Check is not failing on real code.

I merged, squashed and created a new branch issue-6311 on my fork.
This line above shows me that you believe that we use checkstyle including my new OrderedPropertiesCheck somewhere. This is not the case. I was running the diff report against: https://github.com/checkstyle/checkstyle
So the diff report will also show no violations. It looks similar to the one I published to https://thomassenger.github.io/testrepo/.
I have no active project using checkstyle. Let me see what I can do.
Regards.

rnveach · 2019-05-23T22:47:43Z

@thomassenger

I made a different repo: https://thomassenger.github.io/testrepo/

You seemed to have uploaded the raw reports generated by maven plugin and not the ones produced by the diff-report utility.
See http://rveach.no-ip.org/checkstyle/regression/reports/247/ as an example of the report we are expecting to be generated. The report generated should be in like a /reports/diff directory.

This line above shows me that you believe that we use checkstyle including my new OrderedPropertiesCheck somewhere. This is not the case.
So the diff report will also show no violations. It looks similar to the one I published to

You must modify the XML config file you give to diff.groovy to include your new check so it is run against different code bases and we can verify it behaves correctly with the violations it produces.
Looking at https://thomassenger.github.io/testrepo/reports/master/guava/checkstyle-checker.xml , the file has not been modified.

I was running the diff report against: https://github.com/checkstyle/checkstyle

As discussed in the readme you must modify projects-to-test-on.properties to include all projects in the file, so you will be running regression against many projects then just the main checkstyle repo.

Sorry this process hasn't worked well for you. We welcome any additions to the readme to make this process more seamless.

thomassenger · 2019-05-24T07:06:51Z

Thank you rnveach for your help. Now its is getting clear.
Now I am running the diff.groovy with more and more projects commented out (projects-to-test-on.properties)
Currently I add also some check rules to my_check.xml.
It is deployed to https://thomassenger.github.io/testrepo/ (I move it later)

If I add my checker module name="OrderedProperties" to my_check.xml the groovy script will fail. I believe because of the missing check at the upstream/master branch.

rnveach · 2019-05-24T10:39:12Z

If I add my checker module name="OrderedProperties" to my_check.xml the groovy script will fail. I believe because of the missing check at the upstream/master branch.

You must run a patch-only difference report because the check doesn't exist in master for a full difference. The report example I gave is similar.
https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation
See the section that says To generate the report only for the patch branch which contains your changes. You need to use --patchBranch and --patchConfig and --mode single.

…ster branch

thomassenger · 2019-05-24T11:44:42Z

I made a new diff report having --patchConfig and --mode single.
The report was pushed to github.com:thomassenger/checkstyle.git master branch
I open a new pull request on branch "issue-6311"
I think is is getting in the right direction.
Regards.

…ster branch

rnveach · 2019-05-24T13:17:13Z

@thomassenger I don't know what happened but it looks like you pushed the diff report to this PR and overwrote your original work.
b829c24 was your original commit

rnveach · 2019-05-24T13:20:10Z

It looks like this PR was moved to #6781 .
@thomassenger in the future, it is best to put the difference report in some place other than the normal repo you use to make PRs with. I will close this PR and we can use the fresh new one and mention this is where the report is located.

thomassenger · 2019-05-24T17:36:03Z

It looks like this PR was moved to #6781 .

Yes, this is what I understand.

@thomassenger in the future, it is best to put the difference report in some place other than the normal repo you use to make PRs with. I will close this PR and we can use the fresh new one and mention this is where the report is located.

You confuse me. That is why I used the testrepo in the first place.
To get everything right I moved all the code to a branch "issue-6311" and made a new PR to have the master branch free for the diff report.
The diff report is: https://thomassenger.github.io/checkstyle/

Anyhow. Lets get this done.
Regards.
Thomas

rnveach · 2019-05-24T17:48:58Z

Thanks. It has been a while since I used github.io . I thought I used a different repo.

This comment has been minimized.

Sign in to view

rnveach requested changes Jan 7, 2019

View reviewed changes

rnveach requested changes Jan 9, 2019

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

romani added the incomplete label Jan 13, 2019

romani removed the incomplete label Jan 27, 2019

rnveach self-assigned this Jan 27, 2019

thomassenger force-pushed the master branch from 1d1d781 to 9b17014 Compare February 5, 2019 15:22

This comment has been minimized.

Sign in to view

thomassenger force-pushed the master branch from 9b17014 to 0b94c26 Compare February 5, 2019 19:46

This comment has been minimized.

Sign in to view

rnveach requested changes Feb 20, 2019

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java Outdated Show resolved Hide resolved

src/main/java/com/puppycrawl/tools/checkstyle/checks/OrderedPropertiesCheck.java Outdated Show resolved Hide resolved

thomassenger force-pushed the master branch from 874fe31 to f37047f Compare February 21, 2019 07:19

thomassenger force-pushed the master branch from 7b074a8 to 7c54394 Compare April 29, 2019 11:02

romani requested changes May 3, 2019

View reviewed changes

romani removed their assignment May 5, 2019

rnveach assigned romani May 6, 2019

romani requested changes May 11, 2019

View reviewed changes

thomassenger force-pushed the master branch from b2d2887 to b829c24 Compare May 23, 2019 19:39

Issue checkstyle#6311: OrderedPropertiesCheck place diff.groovy to ma…

de67f4a

…ster branch

thomassenger force-pushed the master branch from b829c24 to de67f4a Compare May 24, 2019 11:37

Issue checkstyle#6311: OrderedPropertiesCheck place diff.groovy to ma…

eb6573a

…ster branch

rnveach closed this May 24, 2019

rnveach mentioned this pull request May 24, 2019

Issue #6311: OrderedPropertiesCheck check that properties are ordered #6781

Merged

romani mentioned this pull request Jun 7, 2022

minor: use iterator instead of enumeration #11702

Merged

Issue: #6311 new check: OrderedProperties #6343

Issue: #6311 new check: OrderedProperties #6343

Conversation

thomassenger commented Jan 3, 2019 • edited by rnveach

rnveach commented Jan 3, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rnveach left a comment

Choose a reason for hiding this comment

rnveach commented Jan 7, 2019

This comment has been minimized.

thomassenger commented Jan 13, 2019

romani commented Jan 27, 2019

rnveach commented Jan 27, 2019

rnveach commented Feb 1, 2019

thomassenger commented Feb 5, 2019

rnveach commented Feb 5, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rnveach left a comment

Choose a reason for hiding this comment

rnveach left a comment

Choose a reason for hiding this comment

romani commented Feb 22, 2019

thomassenger commented Feb 22, 2019

thomassenger commented Apr 29, 2019

thomassenger commented Apr 30, 2019 • edited

romani left a comment • edited

Choose a reason for hiding this comment

thomassenger commented May 7, 2019

romani left a comment • edited

Choose a reason for hiding this comment

romani commented May 17, 2019

thomassenger commented May 18, 2019

thomassenger commented May 21, 2019 • edited

romani commented May 22, 2019 • edited

romani commented May 22, 2019

romani commented May 22, 2019

thomassenger commented May 23, 2019

rnveach commented May 23, 2019

thomassenger commented May 24, 2019 • edited

rnveach commented May 24, 2019

thomassenger commented May 24, 2019

rnveach commented May 24, 2019 • edited

rnveach commented May 24, 2019

thomassenger commented May 24, 2019

rnveach commented May 24, 2019

thomassenger commented Jan 3, 2019 •

edited by rnveach

rnveach commented Jan 3, 2019 •

edited

thomassenger commented Apr 30, 2019 •

edited

romani left a comment •

edited

romani left a comment •

edited

thomassenger commented May 21, 2019 •

edited

romani commented May 22, 2019 •

edited

thomassenger commented May 24, 2019 •

edited

rnveach commented May 24, 2019 •

edited