Great, thank you for your fast reply.
On Thu, Jun 30, 2016 at 8:29 PM, Mike Percy <[email protected]> wrote: > That worked. GItHub is updated now. > > ASF infra had some issues yesterday, I'm assuming that's why that mirroring > didn't get triggered. Good catch. > > Mike > > On Thu, Jun 30, 2016 at 10:25 AM, Mike Percy <[email protected]> wrote: > > > 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 > >> > >> > > >> > >> > >> > > > >> > > > >> > > >> > > >> > > > > >
