Hi Mike, for some reason your commit is not available on github. Are you aware of that?
On Thu, Jun 30, 2016 at 7:37 AM, Lior Zeno <liorz...@gmail.com> wrote: > 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" <mpe...@apache.org> 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 <mpe...@apache.org> 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 <liorz...@gmail.com> 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 <mpe...@apache.org> wrote: > >> > >> > On Tue, Jun 28, 2016 at 1:46 PM, Lior Zeno <liorz...@gmail.com> > 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 > >> > > >> > > > > > >
