On Tue, Jun 28, 2016 at 1:46 PM, Lior Zeno <[email protected]> wrote:
> A few suggestions if I may:
> 1. Applying the style can be done via your ide. For instance, if you use
> intellij you can reformat the code using a coding style definition. This
> can greatly minimize your work here.
>
I'm using that for files that have like 50+ warnings. Otherwise I am trying
to minimize the diff and do it by hand. It tends to be noisy.
> 2. I think that the "special imports" part, currently set to com.google can
> be removed. It's annoying.
>
Yes, done
> 3. The MagicNumber module can be added, it's pretty useful.
>
Seems OK to me, but initially I'm mostly concerned about the indentation
and spacing being inconsistent as well as extremely long lines. I spend too
much time finding that kind of thing in code reviews. Maybe we can add more
strict rules in a second pass, though?
> Can we discuss the weakened version? What parts did you find too annoying?
Stuff I found less helpful and more annoying: Disallowing static imports
(nice in tests for assertTrue, and friends, also nice for closely related
configuration constants), disallowing single-line if statements (braces
should certainly be required if multiple lines), disallowing declaring
multiple variables on the same line (rarely a source of problems),
requiring Javadoc on all methods (noisy, often not useful), rules about how
to wrap operators across lines (too controversial), bad heuristics on rules
about single-line comment indentation (bad implementation in checkstyle), rules
about variable naming (A lot of existing code uses Exception e / Throwable
t)
Some stuff that I'm not really against but would be too noisy for a first
patch: ordering of imports (different IDEs will do different things by
default)
This is the current diff to the main google style that I am currently
entertaining. It is starting to seem reasonable:
diff --git a/flume-checkstyle/src/main/resources/flume/checkstyle.xml
b/flume-checkstyle/src/main/resources/flume/checkstyle.xml
index 4fa2737..e8913f0 100644
--- a/flume-checkstyle/src/main/resources/flume/checkstyle.xml
+++ b/flume-checkstyle/src/main/resources/flume/checkstyle.xml
@@ -43,14 +43,18 @@
<property name="max" value="100"/>
<property name="ignorePattern" value="^package.*|^import.*|a
href|href|http://|https://|ftp://"/>
</module>
- <module name="AvoidStarImport"/>
+ <module name="AvoidStarImport">
+ <property name="allowStaticMemberImports" value="true"/>
+ </module>
<module name="OneTopLevelClass"/>
<module name="NoLineWrap"/>
<module name="EmptyBlock">
<property name="option" value="TEXT"/>
<property name="tokens" value="LITERAL_TRY, LITERAL_FINALLY,
LITERAL_IF, LITERAL_ELSE, LITERAL_SWITCH"/>
</module>
- <module name="NeedBraces"/>
+ <module name="NeedBraces">
+ <property name="allowSingleLineStatement" value="true"/>
+ </module>
<module name="LeftCurly">
<property name="maxLineLength" value="100"/>
</module>
@@ -70,7 +74,6 @@
value="WhitespaceAround: ''{0}'' is not preceded with
whitespace."/>
</module>
<module name="OneStatementPerLine"/>
- <module name="MultipleVariableDeclarations"/>
<module name="ArrayTypeStyle"/>
<module name="MissingSwitchDefault"/>
<module name="FallThrough"/>
@@ -78,6 +81,9 @@
<module name="ModifierOrder"/>
<module name="EmptyLineSeparator">
<property name="allowNoEmptyLineBetweenFields" value="true"/>
+ <property name="allowMultipleEmptyLines" value="false"/>
+ <property name="allowMultipleEmptyLinesInsideClassMembers"
value="false"/>
+ <property name="tokens" value="IMPORT, CLASS_DEF,
INTERFACE_DEF, ENUM_DEF, STATIC_INIT, INSTANCE_INIT, CTOR_DEF,
VARIABLE_DEF"/>
</module>
<module name="SeparatorWrap">
<property name="tokens" value="DOT"/>
@@ -96,28 +102,6 @@
<message key="name.invalidPattern"
value="Type name ''{0}'' must match pattern ''{1}''."/>
</module>
- <module name="MemberName">
- <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
- <message key="name.invalidPattern"
- value="Member name ''{0}'' must match pattern ''{1}''."/>
- </module>
- <module name="ParameterName">
- <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
- <message key="name.invalidPattern"
- value="Parameter name ''{0}'' must match pattern ''{1}''."/>
- </module>
- <module name="CatchParameterName">
- <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
- <message key="name.invalidPattern"
- value="Catch parameter name ''{0}'' must match pattern
''{1}''."/>
- </module>
- <module name="LocalVariableName">
- <property name="tokens" value="VARIABLE_DEF"/>
- <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
- <property name="allowOneCharVarInForLoop" value="true"/>
- <message key="name.invalidPattern"
- value="Local variable name ''{0}'' must match pattern
''{1}''."/>
- </module>
<module name="ClassTypeParameterName">
<property name="format"
value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/>
<message key="name.invalidPattern"
@@ -152,22 +136,8 @@
<property name="lineWrappingIndentation" value="4"/>
<property name="arrayInitIndent" value="2"/>
</module>
- <module name="AbbreviationAsWordInName">
- <property name="ignoreFinal" value="false"/>
- <property name="allowedAbbreviationLength" value="1"/>
- </module>
<module name="OverloadMethodsDeclarationOrder"/>
- <module name="VariableDeclarationUsageDistance"/>
- <module name="CustomImportOrder">
- <property name="specialImportsRegExp" value="com.google"/>
- <property name="sortImportsInGroupAlphabetically"
value="true"/>
- <property name="customImportOrderRules"
value="STATIC###SPECIAL_IMPORTS###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE"/>
- </module>
<module name="MethodParamPad"/>
- <module name="OperatorWrap">
- <property name="option" value="NL"/>
- <property name="tokens" value="BAND, BOR, BSR, BXOR, DIV,
EQUAL, GE, GT, LAND, LE, LITERAL_INSTANCEOF, LOR, LT, MINUS, MOD,
NOT_EQUAL, PLUS, QUESTION, SL, SR, STAR "/>
- </module>
<module name="AnnotationLocation">
<property name="tokens" value="CLASS_DEF, INTERFACE_DEF,
ENUM_DEF, METHOD_DEF, CTOR_DEF"/>
</module>
@@ -175,22 +145,17 @@
<property name="tokens" value="VARIABLE_DEF"/>
<property name="allowSamelineMultipleAnnotations"
value="true"/>
</module>
- <module name="NonEmptyAtclauseDescription"/>
- <module name="JavadocTagContinuationIndentation"/>
- <module name="SummaryJavadoc">
- <property name="forbiddenSummaryFragments" value="^@return the
*|^This method returns |^A [{]@code [a-zA-Z0-9]+[}]( is a )"/>
- </module>
- <module name="JavadocParagraph"/>
<module name="AtclauseOrder">
<property name="tagOrder" value="@param, @return, @throws,
@deprecated"/>
<property name="target" value="CLASS_DEF, INTERFACE_DEF,
ENUM_DEF, METHOD_DEF, CTOR_DEF, VARIABLE_DEF"/>
</module>
<module name="JavadocMethod">
<property name="scope" value="public"/>
+ <property name="allowMissingJavadoc" value="true"/>
<property name="allowMissingParamTags" value="true"/>
<property name="allowMissingThrowsTags" value="true"/>
<property name="allowMissingReturnTag" value="true"/>
- <property name="minLineCount" value="2"/>
+ <property name="minLineCount" value="0"/>
<property name="allowedAnnotations" value="Override, Test"/>
<property name="allowThrowsTagsForSubclasses" value="true"/>
</module>
@@ -205,6 +170,8 @@
<module name="EmptyCatchBlock">
<property name="exceptionVariableName" value="expected"/>
</module>
- <module name="CommentsIndentation"/>
+ <module name="CommentsIndentation">
+ <property name="tokens" value="BLOCK_COMMENT_BEGIN"/>
+ </module>
</module>
</module>
Mike
