Issue #12716: Make config filenames kebab-case#12755
Conversation
| <suppress id="properCurlCommand" files="release\.sh"/> | ||
|
|
||
| <suppress id="kebabCaseFileNamingInConfigFolder" | ||
| files="config[\\/]StarterRuleSet-AllRulesByCategory\.groovy\.txt"/> |
There was a problem hiding this comment.
This file was dubious. Not sure if I should rename it and what I should rename it to.
starter-rule-set-all-rules-by-category.groovy.txt?
There was a problem hiding this comment.
I am ok to change name of this file
| -Dcheckstyle.configLocation=../../../config/checkstyle_checks.xml \ | ||
| -Dcheckstyle.nonMain.configLocation=../../../config/checkstyle_non_main_files_checks.xml | ||
| -Dcheckstyle.configLocation=../../../config/checkstyle-checks.xml \ | ||
| -Dcheckstyle.nonMain.configLocation=../../../config/checkstyle-non-main-files-checks.xml |
There was a problem hiding this comment.
Why is it not finding the file? Local run fails too..
https://checkstyle.semaphoreci.com/jobs/7fc71ca3-4a6b-4cac-afd7-11ca7d5637b3#L786
cannot initialize module SuppressionFilter - Unable to find: config/checkstyle-non-main-files-suppressions.xml
There was a problem hiding this comment.
I am not seeing why, but this is a maven run of sevntu-checks. It is not a run of Checkstyle.
There was a problem hiding this comment.
mvn -e --no-transfer-progress -Pno-validations verify -Dcheckstyle.ant.skip=false \
-Dcheckstyle.version="${CS_POM_VERSION}" \
-Dcheckstyle.configLocation=../../../config/checkstyle-checks.xml \
-Dcheckstyle.nonMain.configLocation=../../../config/checkstyle-non-main-files-checks.xmlThere was a problem hiding this comment.
It seems odd, the error message doesn't say ../../, so I am thinking something is missing in the transfer.
There was a problem hiding this comment.
I see the issue now. We are passing sevntu our XML configurations so it can run them. However, our config still references our suppression file naming and not sevntu's file naming. Sevntu still has the old name, and our configuration has the new name. Hence it is not finding the file and failing.
@stoyanK7 You must apply similar changes to sevntu just like in other PRs.
There was a problem hiding this comment.
we have hardcoded file:
checkstyle/config/checkstyle_non_main_files_checks.xml
Lines 12 to 15 in a1731ba
we need it to be like:
checkstyle/config/checkstyle_checks.xml
Lines 36 to 38 in a1731ba
So in this case sevntu can stay on names of file it needs to be.
So before mergin this PR we need 1 PR for checkstyle repo to make it as variable and 1 PR for sevnu to define this variable.
sevntu still need to use released version of checkstyle config, so it will take some time before sevntu updated to latest of checkstyle (due to dependency to eclipse-cs)
There was a problem hiding this comment.
@rnveach We have to merge sevntu-checkstyle/sevntu.checkstyle#989 first, before merging this:
https://checkstyle.semaphoreci.com/jobs/2c087901-7776-40ab-9a1a-30350c48a94d#L639
[ERROR] [checkstyle] [ERROR] /home/semaphore/checkstyle/.ci-temp/sevntu.checkstyle/sevntu-checks/config/CheckstyleFormatterForEclipse.xml:1: Filename should be in kebab-case. [kebabCaseFileNamingInConfigFolder]
[ERROR] [checkstyle] [ERROR] /home/semaphore/checkstyle/.ci-temp/sevntu.checkstyle/sevntu-checks/config/checkstyle_non_main_files_suppressions.xml:1: Filename should be in kebab-case. [kebabCaseFileNamingInConfigFolder]
|
@rnveach Ping. This one is ready. |
|
need to make CI green sooner, if any items to improve, please share , we can improve in next PR. |
Part of #12716