Absolutely. I am definitely not proposing that for right away. That's probably a separate discuss thread anyway as we get closer to make sure we air out any concerns and make sure we aren't leaving a lot of PRs with difficult merges and so on.
On Tue, May 9, 2017 at 7:27 PM, Matt Foley <ma...@apache.org> wrote: > Could I ask please that any major reformat wait until the Metron 0.4.0 > release is out? > Such across-the-board changes usually induce instability for a while, no > matter how carefully done. > > Thanks, > --Matt > (with RM hat on) > > On 5/9/17, 4:12 PM, "Justin Leet" <justinjl...@gmail.com> wrote: > > https://github.com/apache/incubator-metron/pull/577 is up, based on > this > discussion. It includes instructions for setting up both warnings in > IntelliJ and autoformatting settings. They're a bit reconstructed > after the > fact, so if anything is wrong, let me know and I'll update them. > > https://issues.apache.org/jira/browse/METRON-747 can follow more or > less > whenever. It's pretty easy to do a bulk reformat of only Java files. > Like > I said before, we probably want to regenerate anything autogenerated > afterwards (i.e. anything resulting from the Stellar grammar), since > I'm > not convinced that they'll actually be ignored. Checkstyle supports a > couple different ways of handling warnings, so the way those files use > might not actually work as expected. I'd rather just blanket format, > regenerated, and then call it. > > I am inclined to not turn on enforcement of anything until we think > through > exactly how we want it to work (e.g. handling just avoiding > introducing new > errors vs just not allowing any errors vs. setting max errors per > module). > I'm not sure how other projects handle this, and we probably want to > look > into it. > > On Tue, May 9, 2017 at 6:07 AM, Justin Leet <justinjl...@gmail.com> > wrote: > > > I'll have a PR up a little later this week once I have a chance to > sit > > down and test/organize notes a little more. > > > > On Mon, May 8, 2017 at 10:04 PM, zeo...@gmail.com <zeo...@gmail.com> > > wrote: > > > >> +1 definitely a good idea > >> > >> On Mon, May 8, 2017, 9:07 PM Michael Miklavcic < > >> michael.miklav...@gmail.com> > >> wrote: > >> > >> > +1 Justin. Thanks. > >> > > >> > On Mon, May 8, 2017 at 2:55 PM, Matt Foley < > mfo...@hortonworks.com> > >> wrote: > >> > > >> > > +1. I originally suggested the Sun style as a starting point, > and I > >> find > >> > > Justin’s arguments convincing, especially if there is a > pre-packaged > >> > Google > >> > > style checking plug-in. > >> > > --Matt > >> > > > >> > > On 5/8/17, 9:47 AM, "Kyle Richardson" < > kylerichards...@gmail.com> > >> wrote: > >> > > > >> > > +1 Thanks, Justin. I'm all for adopting the Google style > guide. > >> > > > >> > > -Kyle > >> > > > >> > > On Mon, May 8, 2017 at 10:24 AM, Justin Leet < > >> justinjl...@gmail.com> > >> > > wrote: > >> > > > >> > > > Sigh, the message was hidden below the fold. > >> > > > > >> > > > Part of what I've been playing around with is doing > this. It's > >> > > pretty easy > >> > > > to set up both autoformatting and warnings in idea. > There's a > >> > > checkstyle > >> > > > plugin, and you can just import the appropriate file on > each. > >> As a > >> > > fair > >> > > > warning, until we do the blanket reformatting there will > be a > >> lot > >> > of > >> > > > warning highlighting in a lot of the Java files. > >> > > > > >> > > > I'll be documenting that as part of the ticket > (regardless of > >> > > checkstyle > >> > > > configuration specifics), and I'll also update the wiki > (or > >> point > >> > to > >> > > links > >> > > > for it, I believe Checkstyle's docs walk through how to > do it) > >> as > >> > > part of > >> > > > the exercise. > >> > > > > >> > > > Sorry about spamming the list. > >> > > > > >> > > > On Mon, May 8, 2017 at 10:21 AM, Justin Leet < > >> > justinjl...@gmail.com> > >> > > > wrote: > >> > > > > >> > > > > Accidentally sent this to only Otto, instead of the > whole > >> group. > >> > > > > > >> > > > > Part of what I've been playing around with is doing > this. > >> It's > >> > > pretty > >> > > > > easy to set up both autoformatting and warnings in idea. > >> > There's a > >> > > > > checkstyle plugin, and you can just import the > appropriate > >> file > >> > on > >> > > each. > >> > > > > As a fair warning, until we do the blanket reformatting > there > >> > will > >> > > be a > >> > > > lot > >> > > > > of warning highlighting in a lot of the Java files. > >> > > > > > >> > > > > I'll be documenting that as part of the ticket > (regardless of > >> > > checkstyle > >> > > > > configuration specifics), and I'll also update the wiki > (or > >> point > >> > > to > >> > > > links > >> > > > > for it, I believe Checkstyle's docs walk through how to > do > >> it) as > >> > > part of > >> > > > > the exercise. > >> > > > > > >> > > > > On Mon, May 8, 2017 at 9:40 AM, Otto Fowler < > >> > > ottobackwa...@gmail.com> > >> > > > > wrote: > >> > > > > > >> > > > >> +1. > >> > > > >> Does anyone have an idea setup for this? > >> > > > >> > >> > > > >> > >> > > > >> On May 8, 2017 at 09:29:15, Justin Leet ( > >> justinjl...@gmail.com) > >> > > wrote: > >> > > > >> > >> > > > >> I've been taking a look at setting up checkstyle per > >> > > > >> https://issues.apache.org/jira/browse/METRON-746. > >> > > > >> > >> > > > >> Given that we don't actually enforce any style right > now > >> (saying > >> > > we use > >> > > > >> Sun's is not the same as enforcing it!), and plan to > do a > >> > blanket > >> > > format > >> > > > >> after we get checkstyle setup, this seems like a good > >> > opportunity > >> > > to > >> > > > >> discuss what we want to actually implement. > >> > > > >> > >> > > > >> In particular, I'm proposing moving to Google's code > style > >> guide > >> > > and > >> > > > away > >> > > > >> from Sun's. > >> > > > >> > >> > > > >> - Google's style guide already address the > (admittedly) minor > >> > > issues we > >> > > > >> have with the Sun Style, in particular two spaces and > line > >> > length > >> > > (100 > >> > > > in > >> > > > >> Google's). I'd prefer to just use something built-in > as much > >> as > >> > > > possible, > >> > > > >> and it seems like Google's style is closer to what > we're > >> looking > >> > > for out > >> > > > >> of > >> > > > >> the box. > >> > > > >> - Not only is it built in, the explanation for each > rule (and > >> > > where > >> > > > >> checkstyle falls short) actually exists. > >> > > > >> - http://checkstyle.sourceforge.net/google_style.html > >> > > > >> - http://checkstyle.sourceforge.net/sun_style.html > >> > > > >> - Storm uses it, so it makes our code's style more > compatible > >> > with > >> > > > >> one of our core dependencies (and one we've had to dig > into > >> > > repeatedly > >> > > > >> during upgrades and such). > >> > > > >> - https://github.com/apache/storm/blob/master/pom.xml# > L1100 > >> > > > >> - I personally find Google's guide easier to read and > digest. > >> > I'm > >> > > > >> curious how other people feel. > >> > > > >> > >> > > > >> For ease of comparison: > >> > > > >> Sun's guide: > >> > http://www.oracle.com/technetwork/java/codeconvtoc- > >> > > 136057. > >> > > > >> html > >> > > > >> Google's guide: https://google.github.io/ > >> > > styleguide/javaguide.html > >> > > > >> > >> > > > >> > >> > > > > > >> > > > > >> > > > >> > > > >> > > > >> > > >> -- > >> > >> Jon > >> > > > > > > > >
