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
Pull #990: Define variable for non-main-files-suppressions file #990
Conversation
@stoyanK7 Are you still working on the PR in main repo? I need to see it with this to verify the whole flow will work. |
@rnveach I will come back to it later today. I suppose you want me to connect it to this PR and see green CI? |
If you can make main repo CI green and show proof it is invoking the new stuff, then that will be even better as it will show it actually works. |
@rnveach To prove that this PR works, I think we actually need to change <checkstyle.nonMain.configLocation>https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-${checkstyle.version}/config/checkstyle_non_main_files_checks.xml</checkstyle.nonMain.configLocation> to <checkstyle.nonMain.configLocation>https://raw.githubusercontent.com/checkstyle/checkstyle/master/config/checkstyle_non_main_files_checks.xml</checkstyle.nonMain.configLocation>
We already provide the variable in main repo: That got merged yesterday: checkstyle/checkstyle#12761 |
checkstyle/checkstyle@40f4092 is what I was looking for. It was merged without my review so i didn't realize. Truthfully, this PR should have been made first, and then a PR in main repo referencing this and adding extra commit to test and show it is hooking in. (checkstyle/contribution#747 as an example) |
I am not sure what I did wrong. What blocks this PR to be merged? |
I think nothing blocks it? |
@@ -21,6 +21,7 @@ | |||
<checkstyle.version>10.7.0</checkstyle.version> | |||
<checkstyle.configLocation>https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-${checkstyle.version}/config/checkstyle_checks.xml</checkstyle.configLocation> | |||
<checkstyle.nonMain.configLocation>https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-${checkstyle.version}/config/checkstyle_non_main_files_checks.xml</checkstyle.nonMain.configLocation> | |||
<checkstyle.non-main-files-suppressions.file>config/checkstyle_non_main_files_suppressions.xml</checkstyle.non-main-files-suppressions.file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming doesn't follow our other names.
checkstyle.nonMain.configLocation
versus checkstyle.non-main-files-suppressions.file
. Camel cases versus ¿kebob? case.
If I ask this to be changed, then another PR to main repo has to be made as the 2 need to connect together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romani I'll leave this to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use for properties camelcase as we now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But see how badly inconsistent we are on property names... .
Let's do this in separate PR.
@stoyanK7 It seems there is issues with the travis runs and it is actually failing. This PR just had a 10 minute time out run and it was not confirmed to be working. Another PR is failing. https://app.travis-ci.com/github/sevntu-checkstyle/sevntu.checkstyle/jobs/597291051
|
@rnveach On it. That was expected after the release happened. |
From checkstyle/checkstyle#12755 (comment)
Related to checkstyle/checkstyle#12761