Thanks for the heads-up, Lior. Sounds like an ASF infra bug because it was committed upstream: https://git-wip-us.apache.org/repos/asf?p=flume.git
I will try committing a small change to trunk and see if GItHub picks it up. If that doesn't work, I'll file an INFRA ticket. Mike On Thu, Jun 30, 2016 at 10:00 AM, Lior Zeno <[email protected]> wrote: > 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 <[email protected]> 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" <[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 > > >> > > > >> > > > > > > > > > > >
