flume-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Percy <mpe...@apache.org>
Subject Re: [DISCUSS] Checkstyle maven plugin
Date Tue, 28 Jun 2016 20:42:02 GMT
OK awesome.

I'm working on a version of that file that is weakened. That one is super
strict (and a bit annoying).

I'll try to submit a patch for this in the next couple of days.

Mike

On Tue, Jun 28, 2016 at 12:54 PM, Lior Zeno <liorzino@gmail.com> wrote:

> FLUME-2706, FLUME-2919 and FLUME-2921 are the largest pending patches we
> currently have. But I think it's ok to resubmit those patches, it won't be
> too much work.
>
> Also, are you planning to use google's style?
>
> https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml
> Are you going to use it as is?
>
> On Tue, Jun 28, 2016 at 10:48 PM, Mike Percy <mpercy@apache.org> wrote:
>
> > My preference would be to get this in ASAP because I want to make code
> > reviews less work. I'll make the patch (you are right, it is large) to
> make
> > the existing code comply.
> >
> > Personally I don't know of any huge patches that we are planning to merge
> > for 1.7.0 so I don't really think it will be difficult to adapt existing
> > patches. We're not changing every line across the code base, just the
> lines
> > that don't comply.
> >
> > Do you know of any large patches that you think would have major
> conflicts?
> > Maybe we should try to merge those first.
> >
> > Mike
> >
> > On Tue, Jun 28, 2016 at 12:34 PM, Lior Zeno <liorzino@gmail.com> wrote:
> >
> > > I'm not sure that we should integrate that so close to the release,
> since
> > > it won't only affect next commits, but it would require modifications
> to
> > > our current code that violates the style. Usually this requires a great
> > > deal of changes to many files in the project. I suggest fixing this
> issue
> > > in 1.8.0 (FLUME-2937).
> > >
> > > On Tue, Jun 28, 2016 at 9:56 PM, Mike Percy <mpercy@apache.org> wrote:
> > >
> > > > Thanks everyone for your feedback. It sounds like have have consensus
> > on
> > > > this matter. In that case, I'm going to submit a patch to integrate
> > > > checkstyle into the build.
> > > >
> > > > Mike
> > > >
> > > > On Mon, Jun 27, 2016 at 5:39 PM, Mike Percy <mpercy@apache.org>
> wrote:
> > > >
> > > > > On Mon, Jun 27, 2016 at 4:51 PM, Hari Shreedharan <
> > > > hshreedharan@apache.org
> > > > > > wrote:
> > > > >
> > > > >> I am not sure if a precommit hook will suffice, since we don't
> > > actually
> > > > >> run
> > > > >> pre-commit builds. We will probably need to add it to the full
> build
> > > so
> > > > >> the
> > > > >> developer can figure out the issues before even submitting the
> patch
> > > for
> > > > >> review.
> > > > >>
> > > > >
> > > > > There is a way to get it to run as part of the Maven verify stage,
> > > which
> > > > > happens between the package and install phases [1]. So we wouldn't
> > have
> > > > to
> > > > > run checkstyle when working on unit tests or running one of the
> > tests,
> > > > but
> > > > > checkstyle would have to run to do a full build. It seems to take
> > about
> > > > 20
> > > > > seconds, which is hopefully tolerable but obviously not ideal when
> > all
> > > > you
> > > > > want to do it build the code.
> > > > >
> > > > > I would rather do it as part of a pre-commit build hook, but we're
> > not
> > > > > there yet in terms of build stability or the Jenkins setup. I would
> > > love
> > > > to
> > > > > see that Jenkins infrastructure revived and improved. Gerrit would
> be
> > > > great
> > > > > too. One step at a time. :)
> > > > >
> > > > > Mike
> > > > >
> > > > > [1]
> > > > >
> > > >
> > >
> >
> https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html
> > > > >
> > > > >
> > > >
> > >
> >
>

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