Thank you for this contribution, it is greatly appreciated! I'll create a new issue for a second pass, scheduled to 1.8.0. On Jun 30, 2016 7:31 AM, "Mike Percy" <[email protected]> wrote:
I just committed the first pass, which only includes non-unit test code. I want to do the same exact thing for the unit tests, but I probably won't have time to do it until next week. This is just a decent baseline and additional improvements are welcome. Additional tweaks will likely be much smaller patches. The unified diff for this change was 840KB. Mike On Wed, Jun 29, 2016 at 4:06 PM, Mike Percy <[email protected]> wrote: > 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 >> > >> > >
