Skip to content
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

update to checkstyle 8.18 #197

Merged
merged 2 commits into from Mar 6, 2019
Merged

update to checkstyle 8.18 #197

merged 2 commits into from Mar 6, 2019

Conversation

muhlba91
Copy link
Collaborator

Update CheckStyle to 8.18 as per #196. (Did I pick up all changes in CS correctly?)

@muhlba91
Copy link
Collaborator Author

https://teamcity.jetbrains.com/viewLog.html?buildId=2009968&buildTypeId=Checkstyle_SonarCheckstyleIdeaInspectionsPullRequest&tab=buildLog&_focus=40#_state=32 TC fails although locally I can execute mvn clean package successfully. Also, nothing regarding versioning, etc. has changed since the last commit which ran through TC perfectly fine. Any ideas why TC fails out of a sudden?

@rnveach
Copy link
Member

rnveach commented Feb 26, 2019

https://teamcity.jetbrains.com/viewLog.html?buildId=2009968&buildTypeId=Checkstyle_SonarCheckstyleIdeaInspectionsPullRequest&tab=buildLog

[Step 1/2] Error reading Maven project: Some problems were encountered while processing the POMs:
[ERROR] Unresolveable build extension: Plugin org.sonarsource.sonar-packaging-maven-plugin:sonar-packaging-maven-plugin:1.17 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.sonarsource.sonar-packaging-maven-plugin:sonar-packaging-maven-plugin:jar:1.17 @
[ERROR] Unknown packaging: sonar-plugin @ com.github.checkstyle:checkstyle-sonar-plugin:[unknown-version], /mnt/agent/work/8fd170840a34c46f/checkstyle-sonar-plugin/pom.xml, line 12, column 14
.

I will try re-running it.

@romani
Copy link
Member

romani commented Feb 28, 2019

@muhlba91 , please update README.md

I updated issue wit details on what we should pay attention.

did you test updated Checks and new Filter on your local ? to make sure they works.
I usually do migration myself and make some log of my actions to copy problems and share them in issue (example #189) it make it easy for users just google error and get instruction on how to fix problem.
In the same time I confirm that all metadata is valid and Check is functioning.

@romani
Copy link
Member

romani commented Feb 28, 2019

Any ideas why TC fails out of a sudden?

I restarted one more time, did not help.
I created support request: https://teamcity-support.jetbrains.com/hc/en-us/requests/2001725

I can not try to reproduce it on master as TC show message "This is a secondary TeamCity node. Write operations are disabled on this server." , and there is no way to run a build manually.

@muhlba91
Copy link
Collaborator Author

muhlba91 commented Mar 4, 2019

I have updated the README and changed URLs pointing to any checkstyle URL to use https.
Also, checks are tested locally (fresh instance) - no exceptions or errors. However, when trying to add the new filter (I used the examples shown in http://checkstyle.sourceforge.net/config_filters.html#SuppressionXpathSingleFilter), I don't receive any exception but it seems to get ignored because I still see the warnings for MyMethod in Sonar.

@romani
Copy link
Member

romani commented Mar 5, 2019

Thanks a lot for update, but urls are not valid, please review mapping at checkstyle/checkstyle#6478 , it would be better to separate URL update to separate commit, you can split changes by git add -p easily.

Thanks a lot for your help.

@romani
Copy link
Member

romani commented Mar 5, 2019

I don't receive any exception but it seems to get ignored because I still see the warnings for MyMethod in Sonar.

Test is easy, get violation on some code, try to use filter, if violation disappear it means Filter works.

@muhlba91
Copy link
Collaborator Author

muhlba91 commented Mar 5, 2019

@romani I updated the URLs as described in checkstyle/checkstyle#6478 (hence, replaced http://checkstyle.sourceforge.net/dtds with https://checkstyle.org/dtds and http://www.puppycrawl.com/dtds with https://checkstyle.org/dtds) and separated them into one commit.

Test is easy, get violation on some code, try to use filter, if violation disappear it means Filter works.

That's what I tried with enabling the MethodName check. My test class contained

public class MyTestClass {
  public void MyMethod1() {}
}

which results in a wrong method naming. Then I applied the filter according to http://checkstyle.sourceforge.net/config_filters.html#SuppressionXpathSingleFilter:

<module name="SuppressionXpathSingleFilter">
  <property name="checks" value="MethodName"/>
  <property name="message" value="MyMethod[0-9]"/>
</module>

and analysis resulted in the same error/warning.

@romani
Copy link
Member

romani commented Mar 6, 2019

and analysis resulted in the same error/warning.

it could mean that there might be problem in xpath query, in Filter iteself or in sonar plugin.
can suppress such violation by our CLI ? https://checkstyle.org/cmdline.html#Download_and_Run

it is better to share test cases by bug report template - https://checkstyle.org/report_issue.html#How_to_report_a_bug.3F , it helps quickly reproduce your case and investiagate it, please do.

if suppression works fine in CLI, it is clear problem of plugin. Please make sure you placed filter in Treewalker filters in sonar ui.

@romani romani merged commit a973a2f into checkstyle:master Mar 6, 2019
@romani
Copy link
Member

romani commented Mar 6, 2019

I am merged your PR to simplify code changes, but we before release we need to be sure that new functionality works, it will be not professional to release not working features.
Please share details on how you did testing, I will find laptop time to help you.

@muhlba91
Copy link
Collaborator Author

muhlba91 commented Mar 6, 2019

@romani I can confirm a working filter now. I have accidentially enabled a method name filter from a different SQ plugin which resulted in the same error and confused me when testing the filter.

For your reference, here is my test data:
FileOne.java

public class FileOne {                                                                                                                              
    public void MyMethod1() { } // should not raise method name violation                                                                       }

checkstyle configuration: checkstyle.txt
Running checkstyle results in

> java -Duser.language=en -Duser.country=US -jar checkstyle-8.18-all.jar -c checkstyle.xml FileOne.java
Starting audit...
Audit done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants