Justin, Just to clarify, the default Sun conventions don't abide fully to our style guide, correct? As you mention above, we would need to create a custom checkstyle.xml to handle "extended the character limit of a line past 80 and made it two space indents"
On Tue, Feb 21, 2017 at 3:22 PM, Justin Leet <justinjl...@gmail.com> wrote: > I'm also +1 to blanket formatting. > > As a note, we'll also need to set most of the sun checkstyle violations to > warnings and only fail the build on error. Even the quick and dirty > reformat I tried only got rid of several thousand violations, leaving well > over 10k violations. We'll need to set it to only fail code styling > violations. If we're clever, we could probably even set it up so that test > classes don't even throw warnings on things like magic numbers, but that's > not at all necessary for right now. > > The current PR also does not include a custom checkstyle.xml. It's just > reporting on the default Sun conventions as a start. We can pretty easily > take that and modify it, though. I'd also say we should include the > IntelliJ codestyle template along with instructions for installing in that > portion of effort. I could be pretty easily persuaded to include it in this > PR also though, if we want. > > That checking is not part of the current PR (checkstyle is only in the > reporting element, not the build element). It's easy enough to setup > though. That setup is discussed at maven-checkstyle-plugin > <https://maven.apache.org/plugins/maven-checkstyle-plugin/usage.html>. > > > On Tue, Feb 21, 2017 at 2:13 PM, Otto Fowler <ottobackwa...@gmail.com> > wrote: > > > I would also like the idea configuration. > > > > > > On February 21, 2017 at 17:06:09, Casey Stella (ceste...@gmail.com) > wrote: > > > > +1 to blanket reformat as well. > > > > On Tue, Feb 21, 2017 at 1:25 PM, Otto Fowler <ottobackwa...@gmail.com> > > wrote: > > > > > +1. I agree with Michael’s points. > > > > > > > > > On February 21, 2017 at 16:23:21, Michael Miklavcic ( > > > michael.miklav...@gmail.com) wrote: > > > > > > +1 to a blanket reformat, failed build for improper formatting, and > > > automated formatting. I strongly prefer to remove "thinking" from my > code > > > formatting and it has worked very well for me on large projects in the > > > past. There is capability now in IntelliJ to work with Checkstyle as > > well. > > > https://youtrack.jetbrains.com/issue/IDEA-61520#comment=27-1292600 > > > https://plugins.jetbrains.com/idea/plugin/1065-checkstyle-idea > > > > > > A quick search didn't yield any obviously robust tools for automating > the > > > formatting other than an older non-maintained project named Jalopy. I > > think > > > the checkstyle integration with IntelliJ and Eclipse should suffice > since > > > the Maven plugin would give devs the ability to run checks locally and > in > > > Github via Travis. > > > > > > > > > On Tue, Feb 21, 2017 at 12:32 PM, Nick Allen <n...@nickallen.org> > wrote: > > > > > > > I would be in favor of a blanket, reformat. Whether that is for the > > > entire > > > > code base or one project at a time. Might be able to conquer and > divide > > > > some of the heavy-lifting of testing, if we do a project at a time. > But > > > > whichever way you think is easier. I'd be glad to help. > > > > > > > > On Tue, Feb 21, 2017 at 1:57 PM, Justin Leet <justinjl...@gmail.com> > > > > wrote: > > > > > > > > > I already tried a blanket, manual reformat the other day, through > > > > > IntelliJ. I did every file matching *.java in the project and it > was > > > > > pretty quick. I didn't validate everything looked perfect > afterwards, > > > > but I > > > > > did click into a few files and things looked fine. I'm not quite > sure > > > > what > > > > > the lifecycle of our autogenerated stuff is, so we'd want to regen > > > > > afterwards, but it's a pretty trivial thing to do. > > > > > > > > > > I'm sure there's more nuance (and definitely more testing) than > that, > > > but > > > > > off the top of my head I'm not sure what it would be. Either way, I > > > don't > > > > > think there's a huge amount of effort to just do the reformat, but > > we'd > > > > > still want to spin everything up and test it and so on. It's > probably > > > > more > > > > > work for everybody to rebase onto the (vastly) reformatted code > than > > > > > anything else, which will vary pretty significantly. > > > > > > > > > > For (slight) context, the changes are enough to eliminate ~5k > > > checkstyle > > > > > warnings (and there might be more if we have to tweak anything in > the > > > > code > > > > > formatting). > > > > > > > > > > On Tue, Feb 21, 2017 at 10:34 AM, Casey Stella <ceste...@gmail.com > > > > > > wrote: > > > > > > > > > > > Any idea, with those modifications to checkstyle, how much effort > > it > > > > will > > > > > > take to reformat the code to conform? > > > > > > > > > > > > On Tue, Feb 21, 2017 at 8:23 AM, Justin Leet < > > justinjl...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > As part of: > > > > > > > https://issues.apache.org/jira/browse/METRON-726 > > > > > > > https://github.com/apache/incubator-metron/pull/459 > > > > > > > > > > > > > > I integrated checkstyle into the mvn:site command, and have > > > > checkstyle > > > > > > > reports being run as part of the mvn:site reporting. I expect > to > > be > > > > > > > celebrating hitting 25k checkstyle warnings soon. > > > > > > > > > > > > > > I tested out creating a code formatting setup in IntelliJ, > with a > > > > > couple > > > > > > > slight modifications of the default Sun conventions (extended > the > > > > > > character > > > > > > > limit of a line past 80 and made it two space indents). Given > > that > > > > > > > checkstyle includes it as a default option, it's probably > > > reasonably > > > > > > close > > > > > > > to the Sun conventions. I'm thinking we probably also at least > > > create > > > > > an > > > > > > > Eclipse profile, to open up ease of development. > > > > > > > > > > > > > > There's probably also a discussion about how exactly we want to > > > > enforce > > > > > > it. > > > > > > > Is it just something we add to the PR checklist and have > > reviewers > > > > > give a > > > > > > > glance, do we setup a hook to autoformat code, etc? > > > > > > > > > > > > > > Justin > > > > > > > > > > > > > > > > > > > > > > > > > > > >
