I am about to post a first revision of the patch. I was able to make the changes in a safe manner, and I was able to verify that the generated Java bytecode before and after my (gigantic) patch are the same (after stripping line numbers from the class files). That will make this huge patch far easier to review.
Because of that, there are a lot of things I couldn't fix in the first pass, like naming and a couple cases of long lines (breaking up strings into multiple pieces changes the bytecode). So I added some more suppressions. Still, let me address these comments inline... On Wed, Jun 29, 2016 at 11:26 AM, Lior Zeno <[email protected]> wrote: > It's absolutely fine to have two passes for this. > > Still, a few notes: > 1. Why do we need static star imports? > They are nice to use for asserts in unit tests and importing constants from a configuration-only class. This is a small personal preference of mine, if most people hate it then I am not strongly wedded to this style. It's mostly-harmless syntactic sugar if used judiciously. Example: http://junit.sourceforge.net/javadoc/org/junit/Assert.html 2. allowSingleLineStatement=true is too coarse grained than needed here. > See: http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces. > There isn't anything else offered by the tool. Doesn't seem too bad to me to also allow other one-liners, such as: while (running()) {}, etc. > 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. > We can potentially apply it in a second pass. I don't really have anything against it, but I didn't want to have to change that much code up front. It would also change the bytecode. Also, this could be more controversial. > 4. Do we have abbreviations in the code? It's usually better to write > HttpClient and not HTTPClient. > I agree. Again, for the first pass, I didn't want to change the bytecode. > 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? > This just seemed like work. I want to limit the scope of this restyling work as much as possible. I am not currently up for documenting all of the classes in the project. Also, as I mentioned, if you force people to write Javadocs, they will often write useless Javadocs. The Javadocs are really important on the client API, and not really anything else, in my opinion. The client API has decent Javadocs. Mike > > > 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 > > >
