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. 2. I think that the "special imports" part, currently set to com.google can be removed. It's annoying. 3. The MagicNumber module can be added, it's pretty useful.
Can we discuss the weakened version? What parts did you find too annoying? On Tue, Jun 28, 2016 at 11:42 PM, Mike Percy <[email protected]> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > >
