You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Alex Herbert <al...@gmail.com> on 2020/02/18 18:43:48 UTC
[collections] checkstyle
There have been a few PRs recently in collections with simple formatting
errors. These should be picked up by checkstyle to prevent correction
after merge.
The [collections] checkstyle configuration is old. If I replace it with
the Checkstyle version close to the Sun standard [1] then there are a
lot of errors (>3000). With a few rule changes to ignore items the
number of errors is down to about 2000. A lot of these are things that
should be corrected and would be good to have to filter PRs:
JavadocMethod 188
JavadocStyle 517
JavadocType 186
JavadocVariable 177
WhitespaceAround 86
WhitespaceAfter 68
RedundantModifier 40
Indentation 177
Header 31 (for the Apache licence)
I suggest updating the checkstyle config to at least catch these. I can
either use the config that flagged these errors and remove rules that
are not enforced or add these rules to the current config.
I'm thinking a total refresh of the config and then removal of items
that are not to be enforced is the way to go. These can gradually be
reintroduced as the code gets fixed over time (because I don't think I
will be able to do it all any time soon).
Any opinions on this effort?
Alex
[1]
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [collections] checkstyle
Posted by Alex Herbert <al...@gmail.com>.
> On 18 Feb 2020, at 19:04, Gary Gregory <ga...@gmail.com> wrote:
>
> At issue is that each Commons components has its own style and sometimes
> its own custom Checkstyle configuration in its own location. My preference
> is:
> - Store a default Checkstyle, SpotBugs, JApiCmp, and JaCoCo configuration
> in Commons Parent under src/site/resources
> - Avoid bikeshedding for what should be in these above files by picking
> Commons Lang's versions of these files.
> - Since Commons Collections does not define a Checkstyle, it can inherit it
> from Commons Parent
> - Other components can migrate to the Commons Parent configurations at a
> time of their own choosing.
> Gary
Collections does have a checkstyle config, it is just very old.
It is a lot more manageable using the config from [lang] than the strict one adapted from [numbers]. With the current checkstyle version there are 157 errors. If I upgrade checkstyle to a more recent version (plugin 3.1.0 and dependency 8.29) then there are 177 errors.
For a start I’ll grab the [lang] config and fix the code for that. The config from [lang] misses indentation checks and some whitespace checks so I can do these in round 2.
As for bundling this with parent then I think it may require a different project to package all the commons build resources into a jar to maven central. The jar can then be included as a dependency within the checkstyle (etc) configuration or added to the <build> extensions to make it available on the maven classpath [2]. This config could be put into commons-parent. So should this be a new project commons-build-resources?
[1] https://maven.apache.org/ref/3.6.3/maven-model/maven.html#class_extension <https://maven.apache.org/ref/3.6.3/maven-model/maven.html#class_extension>
[2] https://maven.apache.org/guides/mini/guide-maven-classloading.html#Core_Classloader <https://maven.apache.org/guides/mini/guide-maven-classloading.html#Core_Classloader>
>
> On Tue, Feb 18, 2020 at 1:43 PM Alex Herbert <al...@gmail.com>
> wrote:
>
>> There have been a few PRs recently in collections with simple formatting
>> errors. These should be picked up by checkstyle to prevent correction
>> after merge.
>>
>> The [collections] checkstyle configuration is old. If I replace it with
>> the Checkstyle version close to the Sun standard [1] then there are a
>> lot of errors (>3000). With a few rule changes to ignore items the
>> number of errors is down to about 2000. A lot of these are things that
>> should be corrected and would be good to have to filter PRs:
>>
>> JavadocMethod 188
>>
>> JavadocStyle 517
>>
>> JavadocType 186
>>
>> JavadocVariable 177
>>
>> WhitespaceAround 86
>>
>> WhitespaceAfter 68
>>
>> RedundantModifier 40
>>
>> Indentation 177
>>
>> Header 31 (for the Apache licence)
>>
>> I suggest updating the checkstyle config to at least catch these. I can
>> either use the config that flagged these errors and remove rules that
>> are not enforced or add these rules to the current config.
>>
>> I'm thinking a total refresh of the config and then removal of items
>> that are not to be enforced is the way to go. These can gradually be
>> reintroduced as the code gets fixed over time (because I don't think I
>> will be able to do it all any time soon).
>>
>> Any opinions on this effort?
>>
>> Alex
>>
>> [1]
>>
>> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
Re: [collections] checkstyle
Posted by Alex Herbert <al...@gmail.com>.
On 18/02/2020 19:04, Gary Gregory wrote:
> - Avoid bikeshedding for what should be in these above files by picking
> Commons Lang's versions of these files.
I've finished the PR for collections with the new checkstyle based on
lang's configuration with a few additions:
1. I added a check for the Apache licence header. There were quite a lot
of files with different header formatting. rat check is fine with this
but checkstyle requires an exact match. I've just corrected all the
broken headers (probably about 5% of the source files).
2. I added an indentation check. There were nearly 1000 issues here.
I did not use an auto-formatting tool to avoid mangling human readable
layout. Checkstyle is fine with indents that are greater than 4 spaces.
There are a few occasions where human readable layout wrapped to 3
spaces and checkstyle complained:
StringBuffer buf = ...
buf.append(this)
.append(that) ...
I've not bothered here with checkstyle exclusions. I just wrapped to 4
characters instead.
The indentation rule checks case statement indents. The code was using
either 0 or 4 for the indent. Oracle recommend 0. Violations were lower
with 0 as the indent so I went with that.
I've had a once over through all the diffs using the GitHub PR view.
Most diffs are quite small. Some cases of incorrect indents over entire
classes are harder to review. I'll look again later with fresh eyes. It
would be good for a second review. There are a lot of changes.
There are more whitespace rules that could be added to checkstyle. But
for now I think that more changes just make the PR harder to review.
The PR forces checkstyle:check in the default build executed on travis
and checks both main and test sources. It should prevent code formatting
creep over time.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [collections] checkstyle
Posted by Gary Gregory <ga...@gmail.com>.
At issue is that each Commons components has its own style and sometimes
its own custom Checkstyle configuration in its own location. My preference
is:
- Store a default Checkstyle, SpotBugs, JApiCmp, and JaCoCo configuration
in Commons Parent under src/site/resources
- Avoid bikeshedding for what should be in these above files by picking
Commons Lang's versions of these files.
- Since Commons Collections does not define a Checkstyle, it can inherit it
from Commons Parent
- Other components can migrate to the Commons Parent configurations at a
time of their own choosing.
Gary
On Tue, Feb 18, 2020 at 1:43 PM Alex Herbert <al...@gmail.com>
wrote:
> There have been a few PRs recently in collections with simple formatting
> errors. These should be picked up by checkstyle to prevent correction
> after merge.
>
> The [collections] checkstyle configuration is old. If I replace it with
> the Checkstyle version close to the Sun standard [1] then there are a
> lot of errors (>3000). With a few rule changes to ignore items the
> number of errors is down to about 2000. A lot of these are things that
> should be corrected and would be good to have to filter PRs:
>
> JavadocMethod 188
>
> JavadocStyle 517
>
> JavadocType 186
>
> JavadocVariable 177
>
> WhitespaceAround 86
>
> WhitespaceAfter 68
>
> RedundantModifier 40
>
> Indentation 177
>
> Header 31 (for the Apache licence)
>
> I suggest updating the checkstyle config to at least catch these. I can
> either use the config that flagged these errors and remove rules that
> are not enforced or add these rules to the current config.
>
> I'm thinking a total refresh of the config and then removal of items
> that are not to be enforced is the way to go. These can gradually be
> reintroduced as the code gets fixed over time (because I don't think I
> will be able to do it all any time soon).
>
> Any opinions on this effort?
>
> Alex
>
> [1]
>
> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>