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.

Sign up for GitHub

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

[misc] Added new checkstyle rules introduced in recent versions of the Checkstyle tool #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[misc] Added new checkstyle rules introduced in recent versions of the Checkstyle tool #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 46 additions & 5 deletions ...commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
<property name="message" value="No @author tag allowed"/>
</module>

<module name="RegexpSingleline">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disallow trailing white space.

<property name="format" value="\s+$"/>
</module>

<!-- We cannot use this rule as it fails on our license headers, considering them as copy-pastes!
<module name="StrictDuplicateCode"/>
-->
Expand Down Expand Up @@ -87,10 +91,14 @@

<module name="BooleanExpressionComplexity"/>

<!-- This is a good metric but checkstyle doesn't allow to remove JDK's base classes
from the new count thus making this check unusable IMO
<module name="ClassDataAbstractionCoupling"/>
-->
<module name="ClassDataAbstractionCoupling">
<property name="excludedClasses" value="boolean, byte, char, double, float, int, long, short, void,
Boolean, Byte, Character, Double, Float, Integer, Long, Short, Void,
Object, Class, String, StringBuffer, StringBuilder,
ArrayIndexOutOfBoundsException, Exception, RuntimeException, IllegalArgumentException, IllegalStateException, IndexOutOfBoundsException, NullPointerException, Throwable, SecurityException, UnsupportedOperationException,
List, ArrayList, Deque, Queue, LinkedList, Set, HashSet, SortedSet, TreeSet, Map, HashMap, SortedMap, TreeMap,
ConcurrentHashMap, ConcurrentLinkedDeque, ConcurrentLinkedQueue, DelayQueue, FutureTask, LinkedBlockinDeque, LinkedBlockinQueue, Phaser, PriorityBlockingQueue, RejectedExecutionException, SynchronousQueue"/>
</module>

<module name="ClassFanOutComplexity">
<property name="excludedClasses" value="boolean, byte, char, double, float, int, long, short, void,
Expand Down Expand Up @@ -131,6 +139,8 @@

<module name="EmptyStatement"/>

<module name="EqualsAvoidNull"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer "str".equals(var) over var.equals("str")


<module name="EqualsHashCode"/>

<module name="ExecutableStatementCount"/>
Expand All @@ -145,6 +155,8 @@

<!--module name="FinalParameters"/-->

<module name="GenericWhitespace"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace should not be used inside generics markers.


<!-- Avoid finalizers (this will not find violations that contain linebreaks) -->
<module name="RegexpSinglelineJava">
<property name="format" value="((public)|(protected))\s+void\s+finalize\(\s*\)"/>
Expand Down Expand Up @@ -176,7 +188,12 @@

<!--module name="ImportControl"/-->

<!--module name="ImportOrder"/-->
<module name="ImportOrder">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enforce import groups and alphabetical ordering inside each group.

<property name="groups" value="java,javax,org,com"/>
<property name="ordered" value="true"/>
<property name="separated" value="true"/>
<property name="option" value="bottom"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All static imports are placed at the bottom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how these properties affect static imports, but in any case, the current code style (at least for Eclipse) keeps the static imports at the top, before all the regular imports. Is there a good reason to change the position?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current code style (at least for Eclipse) keeps the static imports at the top

Really? Show me. I see that static imports come last, as we can see in existing code.

I don't see how these properties affect static imports

<property name="option" value="bottom"/> does that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sergiu, you added the eclipse.importorder on Jul 19, 2014 so you cannot use it as an argument. The question was why does eclipse.importorder put static imports at the end? As for example, I recently worked on this test and I did organized my imports (Ctrl + Shift + O). I obviously didn't use eclipse.importorder because it's new and I wasn't aware of it but I can see that others did the same in the past here.

I'm not against putting static imports at the bottom, but I want to understand why keeping them at the top is wrong (as it used to be in Eclipse at least without the eclipse.importorder you added).

<property name="option" value="bottom"/> does that.

I can understand that this puts "something" at the bottom but I don't see why "something" is necessarily "static imports". But anyway, this is just the syntax of Eclipse import order files, cryptic for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to comply with our documented Java code style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the IntelliJ IDEA default is to put static imports at the bottom, so I think the current rule exists because Vincent proposed as a standard what was happening in his IDE.

</module>

<module name="Indentation"/>

Expand Down Expand Up @@ -223,6 +240,8 @@

<module name="MemberName"/>

<module name="MethodCount"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At most 100 methods in a class.


<module name="MethodLength"/>

<!-- Allow for UI methods generated by idea -->
Expand All @@ -232,8 +251,14 @@

<module name="MethodParamPad"/>

<module name="MethodTypeParameterName"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generics parameters should be one uppercase letter only.


<!--module name="MissingCtor"/-->

<!--module name="MissingDeprecated"/-->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This forces us to add a JavaDoc, which we don't want. It would be nice if it only worked the other way around, force the presence of the annotation if the javadoc tag is present.


<module name="MissingOverride"/>

<module name="MissingSwitchDefault"/>

<module name="ModifiedControlVariable"/>
Expand All @@ -250,12 +275,20 @@

<module name="NeedBraces"/>

<module name="NestedForDepth">
<property name="max" value="2"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkstyle default is 1, I increased it.

</module>

<module name="NestedIfDepth">
<property name="max" value="2"/>
</module>

<module name="NestedTryDepth"/>

<!--module name="NoClone"/-->

<!--module name="NoFinalizer"/-->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have both clone and finalize methods in some classes. I'd rather enable these two rules and add exceptions where needed, WDYT?


<module name="NoWhitespaceAfter">
<property name="tokens" value="BNOT, DEC, DOT, INC, LNOT, UNARY_MINUS, UNARY_PLUS"/>
</module>
Expand All @@ -264,8 +297,16 @@

<module name="NPathComplexity"/>

<module name="OneStatementPerLine"/>

<module name="OperatorWrap"/>

<module name="OuterTypeFilename"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks that the outer type name and the file name match. Useful for non-public classes, which I think @cjdelisle would like to use.


<module name="OuterTypeNumber"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one top level class allowed in a file.


<module name="PackageAnnotation"/>

<module name="PackageDeclaration"/>

<module name="PackageName"/>
Expand Down