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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > wrote: > > > > > > > > > On Mon, Jun 27, 2016 at 4:51 PM, Hari Shreedharan < > > > > [email protected] > > > > > > 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 > > > > > > > > > > > > > > > > > > > >
