It's absolutely fine to have two passes for this. Still, a few notes: 1. Why do we need static star imports? 2. allowSingleLineStatement=true is too coarse grained than needed here. See: http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces. 3. Why did you remove all naming conventions? You can define a style that allows loop variables and catch variables to be a single token. 4. Do we have abbreviations in the code? It's usually better to write HttpClient and not HTTPClient. 5. Re/ Javadoc. The style enforces having a javadoc only for public methods that are non trivial (more than 3 LOC) and are not overriding a method in a base class or interface. Is it a problem too? Do we have too many non documented public methods?
On Wed, Jun 29, 2016 at 1:47 AM, Mike Percy <[email protected]> wrote: > 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 >
