You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by osiegmar <gi...@git.apache.org> on 2017/09/08 08:42:21 UTC
[GitHub] commons-lang pull request #285: make checkstyle config more portable (no mav...
GitHub user osiegmar opened a pull request:
https://github.com/apache/commons-lang/pull/285
make checkstyle config more portable (no maven coupling)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/osiegmar/commons-lang portable-checkstyle-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-lang/pull/285.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #285
----
commit 01a820275fc7bd66cb699dd081a432c7dde2645e
Author: Oliver Siegmar <ol...@siegmar.de>
Date: 2017-09-08T08:39:40Z
make checkstyle config more portable (no maven coupling)
----
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/285
Not sure. Here's what I just tried.
```
$ git reset --hard
$ git status
On branch pr/285
....
$ vim checkstyle.xml
$ # removed <property name="file" value="checkstyle-suppressions.xml"/> line
$ mvn clean site
```
And then opening the Checkstyle report (via file:///home/.../commons-lang/target/site/index.html) there are no warnings or error in the checkstyle page.
Are you able to spot which step I am missing to reproduce it?
---
[GitHub] commons-lang pull request #285: make checkstyle config more portable (no mav...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-lang/pull/285
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/285
Thank you for updating the pull request.
Tested locally. Using the changes in the pull request I get the same result as we get when the settings are in the Maven settings.
Commenting out the suppression filter in the checkstyle configuration, the report in the web site now gives me **2828** errors. So the suppression filter is working indeed.
Looks good to me, merging it now.
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/285
[](https://coveralls.io/builds/13181719)
Coverage remained the same at 95.194% when pulling **01a820275fc7bd66cb699dd081a432c7dde2645e on osiegmar:portable-checkstyle-config** into **cc94767e7eabdfcf9d1cab1d8d1d8556864394c6 on apache:master**.
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/285
Checked out pull request branch, built with the change, got no checkstyle errors. Then removed the suppression (without re-adding the previous value in pom.xml) and also got the same result, without any errors. Maybe it is aware that the suppression file must be ignored?
+1 for the change. Will wait for others to review :+1:
Documentation about the SuppressionFilter module.
http://checkstyle.sourceforge.net/config_filters.html#SuppressionFilter
Thank you for your contribution @osiegmar
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/285
[](https://coveralls.io/builds/13196392)
Coverage decreased (-0.001%) to 95.2% when pulling **8b21045bac1d5148ea18650313a32bed3a0e167a on osiegmar:portable-checkstyle-config** into **fdf05fa29babe21e64f9a5b268dc8406d449d4f1 on apache:master**.
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/285
[](https://coveralls.io/builds/13196430)
Coverage remained the same at 95.201% when pulling **67830fe24964b0164d0212cbb4f018f15608ff37 on osiegmar:portable-checkstyle-config** into **fdf05fa29babe21e64f9a5b268dc8406d449d4f1 on apache:master**.
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/285
>Does it really generates a site for the current branch?
It should. That is one step in the release process. Probably due to the site using another configuration (under the reporting tag).
https://github.com/osiegmar/commons-lang/blob/01a820275fc7bd66cb699dd081a432c7dde2645e/pom.xml#L711
Could you update the pull request with your original change duplicated in that portion, please?
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by osiegmar <gi...@git.apache.org>.
Github user osiegmar commented on the issue:
https://github.com/apache/commons-lang/pull/285
Removing the SuppressionFilter results in thousands of errors. Maybe you have a stale checkstyle cache file `target/cachefile`?
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by osiegmar <gi...@git.apache.org>.
Github user osiegmar commented on the issue:
https://github.com/apache/commons-lang/pull/285
I haven't checked the configuration of the maven site plugin in detail. Does it really generates a site for the current branch? I doubt that, as I've seen subversion commands in the config...
Anyway, to run checkstyle, just invoke `mvn clean checkstyle:check`.
---
[GitHub] commons-lang issue #285: make checkstyle config more portable (no maven coup...
Posted by osiegmar <gi...@git.apache.org>.
Github user osiegmar commented on the issue:
https://github.com/apache/commons-lang/pull/285
> Could you update the pull request with your original change duplicated in that portion, please?
Done. Thanks for pointing that out!
---