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 > > > > > > > > > > > > > > > > > > > > >
