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 2019/05/21 11:47:49 UTC
[rng][geometry][statistics][numbers] Updating Checkstyle rules
The checkstyle file for all these projects has a common origin (of
[math]?). Checkstyle has advanced since the origin of these checks and
there are many more checks that can be added to maintain the current
coding style.
I've looked at this for RNG starting from a template for the Sun
standards [1].
The code is maintained to high standard and the sun standards need
little modification. Here are the changes:
- Removed FinalParameters
- Removed MagicNumber
- Removed InnerAssignment
- Change line length to 120
- Changed ParameterNumber to only check methods (not constructors)
- Changed NoWhitespaceAfter to remove checks for array initialisers
allowing the whitespace after the { here:
double[] array = new double[] { 1, 2, 3 };
(arguably this could be left at the default Sun coding style to have
'new double[] {1, 2, 3}'.)
- Changed WhitespaceAround to allow empty constructors (for private
utility constructors) and empty types (for marker interfaces)
- Changed VisibilityModifier to allow protected fields
- Changed OperatorWrap to the current checkstyle config
When run over RNG this requires the following changes:
Some method javadoc required a missing end period.
Line length: Some wrapping to 120 characters is required.
Whitespace after: Some updates to change declarations of generics, e.g.
Map<Integer,String> to Map<Integer, String>
3 utility classes should be final. These may be a breaking API changes.
The classes have private constructor so should not be inherited from by
any user. Only one is public the other two are package private.
1 TODO comment is as yet undone.
Indentation: This picked up a few formatting errors.
All the changes are in a new PR [2] so you can view the new additions to
the checkstyle file and the changes to the code that must be made.
I have just appended the additions to the current checkstyle config.
However going forward it may be better to match the order of the
reference checkstyle template provided by checkstyle and then add to
that for commons specific requirements.
Statistics changes:
I tried this on statistics. There are 415 errors, 50 errors if the test
sources are excluded. Most of these in the source are genuine formatting
issues. The only rule that is broken is LocalFinalVariableName which
requires variables to be named '^[a-z][a-zA-Z0-9]*$'.
I do not agree with naming conventions when the code is implementing an
equation so I would either remove this rule or add checkstyle exceptions
file for the two methods where the rule is broken.
See the modifications for statistics in this PR [3]. I've not fixed all
the tests. They are mainly failures due to indentation or whitespace, e.g:
observedCounts[s-1]++;
vs
observedCounts[s - 1]++;
Fixing them would be easy but I am out of time for today.
[1]
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
[2] https://github.com/apache/commons-rng/pull/44
[3] https://github.com/apache/commons-statistics/pull/10
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [rng][geometry][statistics][numbers] Updating Checkstyle rules
Posted by Alex Herbert <al...@gmail.com>.
> On 21 May 2019, at 23:52, Gilles Sadowski <gi...@gmail.com> wrote:
>
> Hi.
>
> Thanks for the CheckStyle update.
>
> I'd say that we enforce it for the "main" parts of the repositories and
> be lenient for the existing unit tests (but try to follow convention for
> new tests).
There is the option to add an exclusions file to checkstyle. The two PRs totally fix [rng] and [statistics] with some exceptions for [statistics]. I will try and get them merged in the next few days.
I can look at PRs for [numbers] and [geometry] for the main source code and exclude the test sources. [geometry] is the biggest deviator from this style and so the rules may have to be relaxed. In this case I’ll comment out rules that are not achieved.
Note:
I will rearrange the checkstyle file to match the order of the default Sun coding conventions file provided by checkstyle. This should make it easier to keep inline with the reference checkstyle template as new features and checks are added (mainly for newer Java features such as lambdas).
>
> Regards,
> Gilles
>
>
> Le mar. 21 mai 2019 à 17:23, Alex Herbert <al...@gmail.com> a écrit :
>>
>> On 21/05/2019 12:47, Alex Herbert wrote:
>>> The checkstyle file for all these projects has a common origin (of
>>> [math]?). Checkstyle has advanced since the origin of these checks and
>>> there are many more checks that can be added to maintain the current
>>> coding style.
>>>
>>> I've looked at this for RNG starting from a template for the Sun
>>> standards [1].
>>>
>>> The code is maintained to high standard and the sun standards need
>>> little modification. Here are the changes:
>>>
>>> - Removed FinalParameters
>>>
>>> - Removed MagicNumber
>>>
>>> - Removed InnerAssignment
>>>
>>> - Change line length to 120
>>>
>>> - Changed ParameterNumber to only check methods (not constructors)
>>>
>>> - Changed NoWhitespaceAfter to remove checks for array initialisers
>>> allowing the whitespace after the { here:
>>>
>>> double[] array = new double[] { 1, 2, 3 };
>>>
>>> (arguably this could be left at the default Sun coding style to have
>>> 'new double[] {1, 2, 3}'.)
>>>
>>> - Changed WhitespaceAround to allow empty constructors (for private
>>> utility constructors) and empty types (for marker interfaces)
>>>
>>> - Changed VisibilityModifier to allow protected fields
>>>
>>> - Changed OperatorWrap to the current checkstyle config
>>>
>>>
>>> When run over RNG this requires the following changes:
>>>
>>> Some method javadoc required a missing end period.
>>>
>>> Line length: Some wrapping to 120 characters is required.
>>>
>>> Whitespace after: Some updates to change declarations of generics, e.g.
>>>
>>> Map<Integer,String> to Map<Integer, String>
>>>
>>> 3 utility classes should be final. These may be a breaking API
>>> changes. The classes have private constructor so should not be
>>> inherited from by any user. Only one is public the other two are
>>> package private.
>>>
>>> 1 TODO comment is as yet undone.
>>>
>>> Indentation: This picked up a few formatting errors.
>>>
>>>
>>> All the changes are in a new PR [2] so you can view the new additions
>>> to the checkstyle file and the changes to the code that must be made.
>>>
>>> I have just appended the additions to the current checkstyle config.
>>> However going forward it may be better to match the order of the
>>> reference checkstyle template provided by checkstyle and then add to
>>> that for commons specific requirements.
>>>
>>>
>>> Statistics changes:
>>>
>>> I tried this on statistics. There are 415 errors, 50 errors if the
>>> test sources are excluded. Most of these in the source are genuine
>>> formatting issues. The only rule that is broken is
>>> LocalFinalVariableName which requires variables to be named
>>> '^[a-z][a-zA-Z0-9]*$'.
>>>
>>> I do not agree with naming conventions when the code is implementing
>>> an equation so I would either remove this rule or add checkstyle
>>> exceptions file for the two methods where the rule is broken.
>>>
>>> See the modifications for statistics in this PR [3]. I've not fixed
>>> all the tests. They are mainly failures due to indentation or
>>> whitespace, e.g:
>>>
>>> observedCounts[s-1]++;
>>> vs
>>> observedCounts[s - 1]++;
>>>
>>>
>>> Fixing them would be easy but I am out of time for today.
>>>
>>>
>>> [1]
>>> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
>>>
>>> [2] https://github.com/apache/commons-rng/pull/44
>>>
>>> [3] https://github.com/apache/commons-statistics/pull/10
>>>
>>>
>> I've looked at [numbers] with the current config. It uses no indentation
>> for case statements. This is recommended by Oracle. So I've updated the
>> [rng] and [statistics] PRs to reflect this.
>>
>> I've not got time to fix numbers but the new checkstyle file finds a lot
>> of legitimate problems with the current formatting.
>>
>> Old checks:
>>
>> [INFO] There are 6 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 15 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 4 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 3 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>> New checks:
>>
>> [INFO] There are 25 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 62 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-complex/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 19 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-complex-streams/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 47 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-primes/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 5 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-quaternion/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 89 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-fraction/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 3 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-angle/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 17 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 18 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 4 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 2 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-numbers/commons-numbers-field/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>> A lot of these are indentation problems, some naming conventions and
>> whitespace around operators. I think they are mostly legitimate errors
>> and the new checks enforce the coding style.
>>
>>
>> [geometry] Only run on the sources (see later).
>>
>> Old checks:
>>
>> First run with the current version (before changing anything):
>>
>> [INFO] There are 2 errors reported by Checkstyle 6.18 with
>> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>> With the new checkstyle 8.20 version it finds a lot more so a good
>> reason to upgrade just checkstyle:
>>
>> [INFO] There are 11 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 3 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 2 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>> New checks:
>>
>> [INFO] There are 49 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 175 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 3 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 9 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 3 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>>
>> Looking at the worst offenders it is a variety of javadoc, redundant
>> modifier, indentation and whitespace around operators. Some of the
>> checks are legitimate, other are a coding style choice.
>>
>>
>> When run on [geometry] including test sources the errors are very high.
>>
>> [INFO] There are 83 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 1047 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 44 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 97 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>> [INFO] There are 333 errors reported by Checkstyle 8.20 with
>> /home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
>> ruleset.
>>
>>
>> So [geometry] would need a fair bit of work to get it to pass this
>> stricter set of checks for the tests. The main source and the tests have
>> been written with a different style around certain formatting
>> constructs. So the checks would have to be updated or dropped to suit
>> the code style. An upgrade of Checkstyle is still useful as the new
>> version finds more errors with the existing checks.
>>
>> I think [numbers] should not take long to fix all the errors.
>>
>> Summary:
>>
>> - [rng] can use a stricter set of checks with little changes
>>
>> - [statistics] can use a stricter set of checks with little changes
>>
>> - [numbers] should consider updating checkstyle and look at enforcing
>> code style with stricter checks
>>
>> - [geometry] should consider updating checkstyle and look at enforcing
>> code style with stricter checks
>>
>>
>> Adding just indentation, javadoc and left/right braces checks will
>> benefit all projects as these find many legitimate errors in the style.
>>
>>
>> Alex
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [rng][geometry][statistics][numbers] Updating Checkstyle rules
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
Thanks for the CheckStyle update.
I'd say that we enforce it for the "main" parts of the repositories and
be lenient for the existing unit tests (but try to follow convention for
new tests).
Regards,
Gilles
Le mar. 21 mai 2019 à 17:23, Alex Herbert <al...@gmail.com> a écrit :
>
> On 21/05/2019 12:47, Alex Herbert wrote:
> > The checkstyle file for all these projects has a common origin (of
> > [math]?). Checkstyle has advanced since the origin of these checks and
> > there are many more checks that can be added to maintain the current
> > coding style.
> >
> > I've looked at this for RNG starting from a template for the Sun
> > standards [1].
> >
> > The code is maintained to high standard and the sun standards need
> > little modification. Here are the changes:
> >
> > - Removed FinalParameters
> >
> > - Removed MagicNumber
> >
> > - Removed InnerAssignment
> >
> > - Change line length to 120
> >
> > - Changed ParameterNumber to only check methods (not constructors)
> >
> > - Changed NoWhitespaceAfter to remove checks for array initialisers
> > allowing the whitespace after the { here:
> >
> > double[] array = new double[] { 1, 2, 3 };
> >
> > (arguably this could be left at the default Sun coding style to have
> > 'new double[] {1, 2, 3}'.)
> >
> > - Changed WhitespaceAround to allow empty constructors (for private
> > utility constructors) and empty types (for marker interfaces)
> >
> > - Changed VisibilityModifier to allow protected fields
> >
> > - Changed OperatorWrap to the current checkstyle config
> >
> >
> > When run over RNG this requires the following changes:
> >
> > Some method javadoc required a missing end period.
> >
> > Line length: Some wrapping to 120 characters is required.
> >
> > Whitespace after: Some updates to change declarations of generics, e.g.
> >
> > Map<Integer,String> to Map<Integer, String>
> >
> > 3 utility classes should be final. These may be a breaking API
> > changes. The classes have private constructor so should not be
> > inherited from by any user. Only one is public the other two are
> > package private.
> >
> > 1 TODO comment is as yet undone.
> >
> > Indentation: This picked up a few formatting errors.
> >
> >
> > All the changes are in a new PR [2] so you can view the new additions
> > to the checkstyle file and the changes to the code that must be made.
> >
> > I have just appended the additions to the current checkstyle config.
> > However going forward it may be better to match the order of the
> > reference checkstyle template provided by checkstyle and then add to
> > that for commons specific requirements.
> >
> >
> > Statistics changes:
> >
> > I tried this on statistics. There are 415 errors, 50 errors if the
> > test sources are excluded. Most of these in the source are genuine
> > formatting issues. The only rule that is broken is
> > LocalFinalVariableName which requires variables to be named
> > '^[a-z][a-zA-Z0-9]*$'.
> >
> > I do not agree with naming conventions when the code is implementing
> > an equation so I would either remove this rule or add checkstyle
> > exceptions file for the two methods where the rule is broken.
> >
> > See the modifications for statistics in this PR [3]. I've not fixed
> > all the tests. They are mainly failures due to indentation or
> > whitespace, e.g:
> >
> > observedCounts[s-1]++;
> > vs
> > observedCounts[s - 1]++;
> >
> >
> > Fixing them would be easy but I am out of time for today.
> >
> >
> > [1]
> > https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
> >
> > [2] https://github.com/apache/commons-rng/pull/44
> >
> > [3] https://github.com/apache/commons-statistics/pull/10
> >
> >
> I've looked at [numbers] with the current config. It uses no indentation
> for case statements. This is recommended by Oracle. So I've updated the
> [rng] and [statistics] PRs to reflect this.
>
> I've not got time to fix numbers but the new checkstyle file finds a lot
> of legitimate problems with the current formatting.
>
> Old checks:
>
> [INFO] There are 6 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 15 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 4 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 3 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
> New checks:
>
> [INFO] There are 25 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 62 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-complex/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 19 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-complex-streams/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 47 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-primes/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 5 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-quaternion/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 89 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-fraction/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 3 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-angle/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 17 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 18 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 4 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 2 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-numbers/commons-numbers-field/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
> A lot of these are indentation problems, some naming conventions and
> whitespace around operators. I think they are mostly legitimate errors
> and the new checks enforce the coding style.
>
>
> [geometry] Only run on the sources (see later).
>
> Old checks:
>
> First run with the current version (before changing anything):
>
> [INFO] There are 2 errors reported by Checkstyle 6.18 with
> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
> With the new checkstyle 8.20 version it finds a lot more so a good
> reason to upgrade just checkstyle:
>
> [INFO] There are 11 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 3 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 2 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
> New checks:
>
> [INFO] There are 49 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 175 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 3 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 9 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 3 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
>
> Looking at the worst offenders it is a variety of javadoc, redundant
> modifier, indentation and whitespace around operators. Some of the
> checks are legitimate, other are a coding style choice.
>
>
> When run on [geometry] including test sources the errors are very high.
>
> [INFO] There are 83 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 1047 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 44 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 97 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
> [INFO] There are 333 errors reported by Checkstyle 8.20 with
> /home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
> ruleset.
>
>
> So [geometry] would need a fair bit of work to get it to pass this
> stricter set of checks for the tests. The main source and the tests have
> been written with a different style around certain formatting
> constructs. So the checks would have to be updated or dropped to suit
> the code style. An upgrade of Checkstyle is still useful as the new
> version finds more errors with the existing checks.
>
> I think [numbers] should not take long to fix all the errors.
>
> Summary:
>
> - [rng] can use a stricter set of checks with little changes
>
> - [statistics] can use a stricter set of checks with little changes
>
> - [numbers] should consider updating checkstyle and look at enforcing
> code style with stricter checks
>
> - [geometry] should consider updating checkstyle and look at enforcing
> code style with stricter checks
>
>
> Adding just indentation, javadoc and left/right braces checks will
> benefit all projects as these find many legitimate errors in the style.
>
>
> Alex
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [rng][geometry][statistics][numbers] Updating Checkstyle rules
Posted by Alex Herbert <al...@gmail.com>.
On 21/05/2019 12:47, Alex Herbert wrote:
> The checkstyle file for all these projects has a common origin (of
> [math]?). Checkstyle has advanced since the origin of these checks and
> there are many more checks that can be added to maintain the current
> coding style.
>
> I've looked at this for RNG starting from a template for the Sun
> standards [1].
>
> The code is maintained to high standard and the sun standards need
> little modification. Here are the changes:
>
> - Removed FinalParameters
>
> - Removed MagicNumber
>
> - Removed InnerAssignment
>
> - Change line length to 120
>
> - Changed ParameterNumber to only check methods (not constructors)
>
> - Changed NoWhitespaceAfter to remove checks for array initialisers
> allowing the whitespace after the { here:
>
> double[] array = new double[] { 1, 2, 3 };
>
> (arguably this could be left at the default Sun coding style to have
> 'new double[] {1, 2, 3}'.)
>
> - Changed WhitespaceAround to allow empty constructors (for private
> utility constructors) and empty types (for marker interfaces)
>
> - Changed VisibilityModifier to allow protected fields
>
> - Changed OperatorWrap to the current checkstyle config
>
>
> When run over RNG this requires the following changes:
>
> Some method javadoc required a missing end period.
>
> Line length: Some wrapping to 120 characters is required.
>
> Whitespace after: Some updates to change declarations of generics, e.g.
>
> Map<Integer,String> to Map<Integer, String>
>
> 3 utility classes should be final. These may be a breaking API
> changes. The classes have private constructor so should not be
> inherited from by any user. Only one is public the other two are
> package private.
>
> 1 TODO comment is as yet undone.
>
> Indentation: This picked up a few formatting errors.
>
>
> All the changes are in a new PR [2] so you can view the new additions
> to the checkstyle file and the changes to the code that must be made.
>
> I have just appended the additions to the current checkstyle config.
> However going forward it may be better to match the order of the
> reference checkstyle template provided by checkstyle and then add to
> that for commons specific requirements.
>
>
> Statistics changes:
>
> I tried this on statistics. There are 415 errors, 50 errors if the
> test sources are excluded. Most of these in the source are genuine
> formatting issues. The only rule that is broken is
> LocalFinalVariableName which requires variables to be named
> '^[a-z][a-zA-Z0-9]*$'.
>
> I do not agree with naming conventions when the code is implementing
> an equation so I would either remove this rule or add checkstyle
> exceptions file for the two methods where the rule is broken.
>
> See the modifications for statistics in this PR [3]. I've not fixed
> all the tests. They are mainly failures due to indentation or
> whitespace, e.g:
>
> observedCounts[s-1]++;
> vs
> observedCounts[s - 1]++;
>
>
> Fixing them would be easy but I am out of time for today.
>
>
> [1]
> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml
>
> [2] https://github.com/apache/commons-rng/pull/44
>
> [3] https://github.com/apache/commons-statistics/pull/10
>
>
I've looked at [numbers] with the current config. It uses no indentation
for case statements. This is recommended by Oracle. So I've updated the
[rng] and [statistics] PRs to reflect this.
I've not got time to fix numbers but the new checkstyle file finds a lot
of legitimate problems with the current formatting.
Old checks:
[INFO] There are 6 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 15 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 4 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 3 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
New checks:
[INFO] There are 25 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 62 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-complex/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 19 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-complex-streams/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 47 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-primes/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 5 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-quaternion/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 89 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-fraction/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 3 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-angle/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 17 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-gamma/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 18 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-combinatorics/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 4 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-arrays/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 2 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-numbers/commons-numbers-field/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
A lot of these are indentation problems, some naming conventions and
whitespace around operators. I think they are mostly legitimate errors
and the new checks enforce the coding style.
[geometry] Only run on the sources (see later).
Old checks:
First run with the current version (before changing anything):
[INFO] There are 2 errors reported by Checkstyle 6.18 with
/home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
With the new checkstyle 8.20 version it finds a lot more so a good
reason to upgrade just checkstyle:
[INFO] There are 11 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 3 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 2 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
New checks:
[INFO] There are 49 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 175 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 3 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 9 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 3 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
Looking at the worst offenders it is a variety of javadoc, redundant
modifier, indentation and whitespace around operators. Some of the
checks are legitimate, other are a coding style choice.
When run on [geometry] including test sources the errors are very high.
[INFO] There are 83 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-core/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 1047 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-euclidean/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 44 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-enclosing/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 97 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-spherical/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
[INFO] There are 333 errors reported by Checkstyle 8.20 with
/home/ah403/git/commons-geometry/commons-geometry-hull/../src/main/resources/checkstyle/checkstyle.xml
ruleset.
So [geometry] would need a fair bit of work to get it to pass this
stricter set of checks for the tests. The main source and the tests have
been written with a different style around certain formatting
constructs. So the checks would have to be updated or dropped to suit
the code style. An upgrade of Checkstyle is still useful as the new
version finds more errors with the existing checks.
I think [numbers] should not take long to fix all the errors.
Summary:
- [rng] can use a stricter set of checks with little changes
- [statistics] can use a stricter set of checks with little changes
- [numbers] should consider updating checkstyle and look at enforcing
code style with stricter checks
- [geometry] should consider updating checkstyle and look at enforcing
code style with stricter checks
Adding just indentation, javadoc and left/right braces checks will
benefit all projects as these find many legitimate errors in the style.
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org