flume-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lior Zeno <liorz...@gmail.com>
Subject Re: [DISCUSS] Checkstyle maven plugin
Date Thu, 30 Jun 2016 17:34:07 GMT
Great, thank you for your fast reply.


On Thu, Jun 30, 2016 at 8:29 PM, Mike Percy <mpercy@apache.org> 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 <mpercy@apache.org> 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 <liorzino@gmail.com> 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 <liorzino@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" <mpercy@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 <mpercy@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 <liorzino@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 <mpercy@apache.org>
> >> wrote:
> >> > >>
> >> > >> > On Tue, Jun 28, 2016 at 1:46 PM, Lior Zeno <liorzino@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
> >> > >> >
> >> > >>
> >> > >
> >> > >
> >> >
> >> >
> >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message