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 >> > >> > >> > >> >> > > >> > > >> > >> > >> > >
