I just did a pull and now I'm reviewing the formatters under etc/. -rw-rw-r-- 1 klund staff 36004 Oct 21 13:36 eclipse-java-google-style.xml -rw-rw-r-- 1 klund staff 110 Sep 19 11:56 eclipseOrganizeImports.importorder -rw-rw-r-- 1 klund staff 20653 Oct 21 13:36 intellij-java-google-style.xml
What's the status of organizing imports? And do we still use eclipseOrganizeImports.importorder for imports in Eclipse? Thanks, Kirk On Fri, Oct 21, 2016 at 1:33 PM, Mark Bretl <asf.mbr...@gmail.com> wrote: > Thanks Jared for the suggestion of Spotless and follow-up work. > > This is now completed and checked into develop. As this does touch many > files, be prepared the next time you pull. > > --Mark > > > > On Fri, Oct 21, 2016 at 1:21 PM, Jared Stewart <jstew...@pivotal.io> > wrote: > > > Done! :) > > > > - Jared > > > On Oct 21, 2016, at 12:27 PM, Mark Bretl <asf.mbr...@gmail.com> wrote: > > > > > > One more time! :) > > > > > > Conflicting files > > > geode-core/src/test/java/org/apache/geode/disttx/ > PRDistTXDUnitTest.java > > > geode-core/src/test/java/org/apache/geode/disttx/ > > PRDistTXWithVersionsDUnitTest.java > > > geode-core/src/test/java/org/apache/geode/internal/cache/execute/ > > PRTransactionDUnitTest.java > > > > > > --Mark > > > > > > On Fri, Oct 21, 2016 at 12:08 PM, Jared Stewart <jstew...@pivotal.io> > > wrote: > > > > > >> I just pulled and rebased onto develop, and force pushed into the > > existing > > >> pull request. It should be clean to merge in now. > > >> > > >> Thanks, > > >> Jared > > >>> On Oct 21, 2016, at 11:57 AM, Mark Bretl <asf.mbr...@gmail.com> > wrote: > > >>> > > >>> I believe there is enough consensus here to check this into develop. > > >>> > > >>> Jared, due to recent checkins into develop, can you update the pull > > >> request > > >>> one more time? Trying to make this as clean as possible. I will check > > >> into > > >>> develop after the update, unless someone else gets to it first. > > >>> > > >>> All, can we hold checkins on develop until the new formatter is > > applied? > > >>> > > >>> Thanks, > > >>> > > >>> --Mark > > >>> > > >>> On Fri, Oct 21, 2016 at 9:03 AM, Kenneth Howe <kh...@pivotal.io> > > wrote: > > >>> > > >>>> +1 > > >>>> > > >>>>> On Oct 21, 2016, at 8:27 AM, Bruce Schuchardt < > > bschucha...@pivotal.io> > > >>>> wrote: > > >>>>> > > >>>>> +1 > > >>>>> > > >>>>> Le 10/20/2016 à 5:13 PM, Udo Kohlmeyer a écrit : > > >>>>>> +1 > > >>>>>> > > >>>>>> > > >>>>>> On 20/10/16 4:56 pm, Mark Bretl wrote: > > >>>>>>> +1 as well... > > >>>>>>> > > >>>>>>> - Pulled changes > > >>>>>>> - Executed './gradlew clean build' and was successful. > > >>>>>>> - Modified a couple of random files to test > > >>>>>>> - Ran './gradlew clean build' again and failed expectedly > > >>>>>>> - Ran './gradlew spotlessApply', task was successful > > >>>>>>> - Ran './gradlew clean build' and succeeded > > >>>>>>> > > >>>>>>> Great addition! As long as others are good with the formatter, > > then I > > >>>> am > > >>>>>>> good. > > >>>>>>> > > >>>>>>> --Mark > > >>>>>>> > > >>>>>>> On Thu, Oct 20, 2016 at 3:40 PM, Kirk Lund <kl...@apache.org> > > wrote: > > >>>>>>> > > >>>>>>>> +1 I just added my approval to the PR (and again here) > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On Thu, Oct 20, 2016 at 3:25 PM, Jared Stewart < > > jstew...@pivotal.io > > >>> > > >>>>>>>> wrote: > > >>>>>>>> > > >>>>>>>>> I have opened a pull request here <https://github.com/apache/ > > >>>>>>>>> incubator-geode/pull/268> to enable the Spotless plugin and to > > >>>> switch to > > >>>>>>>>> the Google Java Style formatter templates. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>>> On Oct 18, 2016, at 4:32 PM, Kirk Lund <kl...@pivotal.io> > > wrote: > > >>>>>>>>>> > > >>>>>>>>>> For reference TRAC #38741 was a bug with the summary > > "EOFException > > >>>>>>>> during > > >>>>>>>>>> deserialize on client update with copy-on-read=true" > > >>>>>>>>>> > > >>>>>>>>>> -Kirk > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> On Tue, Oct 18, 2016 at 4:27 PM, Jared Stewart < > > >> jstew...@pivotal.io > > >>>>> > > >>>>>>>>> wrote: > > >>>>>>>>>>> To give everyone an update, using the Google Java Style > eclipse > > >>>>>>>> template > > >>>>>>>>>>> there is an issue spotlessCheck where fails for > > >>>>>>>>>>> geode-core/src/test/java/org/apache/geode/cache30/ > > >>>>>>>>> Bug38741DUnitTest.java > > >>>>>>>>>>> even if you run it directly after spotlessApply. This needs > to > > be > > >>>>>>>>>>> investigated and fixed before I can open a pull request to > > enable > > >>>>>>>>> spotless. > > >>>>>>>>>>> > > >>>>>>>>>>>> On Oct 14, 2016, at 4:57 PM, Dan Smith <dsm...@pivotal.io> > > >> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>> +1 - The formatting looks better now. > > >>>>>>>>>>>> > > >>>>>>>>>>>> -Dan > > >>>>>>>>>>>> > > >>>>>>>>>>>> On Thu, Oct 13, 2016 at 11:06 AM, Jared Stewart < > > >>>> jstew...@pivotal.io > > >>>>>>>>>>> wrote: > > >>>>>>>>>>>>> I agree that the formatter needs fixing up. Our wiki < > > >>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/GEODE/Code+ > > >>>> Style+Guide> > > >>>>>>>>>>> says > > >>>>>>>>>>>>> that we follow the Google Java Style guide, but that is not > > >>>> actually > > >>>>>>>>>>> what’s > > >>>>>>>>>>>>> in our formatter templates. I pushed a new branch < > > >>>>>>>>> https://github.com/ > > >>>>>>>>>>>>> jaredjstewart/incubator-geode/tree/ > > spotlessPluginGoogleStyle> > > >>>> that > > >>>>>>>>>>> points > > >>>>>>>>>>>>> spotless at the actual Google Java Style template, and I > > think > > >> it > > >>>>>>>>> makes > > >>>>>>>>>>>>> both of the examples you found look better. > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> On Oct 13, 2016, at 10:11 AM, Dan Smith < > dsm...@pivotal.io> > > >>>> wrote: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> +1 for adding this to ./gradlew build > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> But I think we might want to fix up the formatter a bit > > before > > >>>>>>>>>>>>> reformatting > > >>>>>>>>>>>>>> the code. I tried running spotlessApply, and it did some > > >>>>>>>> unfortunate > > >>>>>>>>>>>>>> reformatting of code to make it less readable. > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> One problem is with method chaining. We have a few > different > > >>>>>>>> factory > > >>>>>>>>>>> APIs > > >>>>>>>>>>>>>> that encourage method chaining, and it put all the method > > >> calls > > >>>> on > > >>>>>>>> a > > >>>>>>>>>>>>> single > > >>>>>>>>>>>>>> line. For example here's one change: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> - ClientCacheFactory ccf = new ClientCacheFactory() > > >>>>>>>>>>>>>> - > > >>>>>>>>>>>>>> .addPoolServer(NetworkUtils.getServerHostName(server1. > > >>>> getHost()), > > >>>>>>>>>>> port) > > >>>>>>>>>>>>>> - .set(SECURITY_CLIENT_AUTH_INIT, > > >>>>>>>>>>>>>> UserPasswordAuthInit.class.getName() + ".create") > > >>>>>>>>>>>>>> - .set(SECURITY_PREFIX+"username", "root") > > >>>>>>>>>>>>>> - .set(SECURITY_PREFIX+"password", "root"); > > >>>>>>>>>>>>>> + ClientCacheFactory ccf = new > > >>>>>>>>>>>>>> ClientCacheFactory().addPoolServer(NetworkUtils. > > >>>>>>>>>>>>> getServerHostName(server1.getHost()), > > >>>>>>>>>>>>>> port).set(SECURITY_CLIENT_AUTH_INIT, > > >>>> UserPasswordAuthInit.class. > > >>>>>>>>>>> getName() > > >>>>>>>>>>>>> + > > >>>>>>>>>>>>>> ".create").set(SECURITY_PREFIX + "username", > > >>>>>>>>>>> "root").set(SECURITY_PREFIX > > >>>>>>>>>>>>> + > > >>>>>>>>>>>>>> "password", "root"); > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> I see a similar problem where it put array initialization > > all > > >>>> on a > > >>>>>>>>>>> single > > >>>>>>>>>>>>>> line: > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> + public void testMultiColOrderByWithIndexRe > > >>>> sultWithProjection() > > >>>>>>>>>>> throws > > >>>>>>>>>>>>>> Exception { > > >>>>>>>>>>>>>> String queries[] = { > > >>>>>>>>>>>>>> // Test case No. IUMR021 > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID desc, pkid desc ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID asc, pkid asc ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 and ID < 20 order by ID asc, pkid asc ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 and ID < 20 order by ID desc , pkid desc", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID >= 10 and ID <= 20 order by ID desc, pkid asc ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID >= 10 and ID <= 20 order by ID asc, pkid desc", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID != 10 order by ID asc , pkid desc", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID != 10 order by ID desc, pkid asc ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID desc, pkid desc limit 5", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID asc, pkid asc limit 5", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 and ID < 20 order by ID asc, pkid desc > limit 5 > > >> ", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 and ID < 20 order by ID desc, pkid asc limit > > 5", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID >= 10 and ID <= 20 order by ID desc, pkid desc > > limit > > >>>> 5", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID >= 10 and ID <= 20 order by ID asc, pkid asc > limit > > >> 5", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID != 10 order by ID asc , pkid desc limit 10", > > >>>>>>>>>>>>>> - "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID != 10 order by ID desc, pkid desc limit 10", > > >>>>>>>>>>>>>> - > > >>>>>>>>>>>>>> - }; > > >>>>>>>>>>>>>> + "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID desc, pkid desc ", "SELECT ID, > > >>>>>>>>> description, > > >>>>>>>>>>>>>> createTime, pkid FROM /portfolio1 pf1 where ID > 10 order > by > > >> ID > > >>>>>>>> asc, > > >>>>>>>>>>> pkid > > >>>>>>>>>>>>>> asc ", "SELECT ID, description, createTime, pkid FROM > > >>>> /portfolio1 > > >>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 and ID < 20 order by ID asc, pkid asc ", > > "SELECT > > >>>>>>>> ID, > > >>>>>>>>>>>>>> description, createTime, pkid FROM /portfolio1 pf1 where > ID > > > > > >> 10 > > >>>>>>>> and > > >>>>>>>>>>> ID < > > >>>>>>>>>>>>>> 20 order by ID desc , pkid desc", "SELECT ID, > description, > > >>>>>>>>>>> createTime, > > >>>>>>>>>>>>>> pkid FROM /portfolio1 pf1 where ID >= 10 and ID <= 20 > order > > by > > >>>> ID > > >>>>>>>>> desc, > > >>>>>>>>>>>>>> pkid asc ", "SELECT ID, description, createTime, pkid > FROM > > >>>>>>>>>>> /portfolio1 > > >>>>>>>>>>>>>> pf1 where ID >= 10 and ID <= 20 order by ID asc, pkid > desc", > > >>>>>>>> "SELECT > > >>>>>>>>>>>>> ID, > > >>>>>>>>>>>>>> description, createTime, pkid FROM /portfolio1 pf1 where > ID > > != > > >>>> 10 > > >>>>>>>>> order > > >>>>>>>>>>>>> by > > >>>>>>>>>>>>>> ID asc , pkid desc", "SELECT ID, description, > createTime, > > >> pkid > > >>>>>>>> FROM > > >>>>>>>>>>>>>> /portfolio1 pf1 where ID != 10 order by ID desc, pkid asc > ", > > >>>>>>>>>>>>>> + "SELECT ID, description, createTime, pkid FROM > > >>>>>>>> /portfolio1 > > >>>>>>>>>>> pf1 > > >>>>>>>>>>>>>> where ID > 10 order by ID desc, pkid desc limit 5", > "SELECT > > >>>> ID, > > >>>>>>>>>>>>>> description, createTime, pkid FROM /portfolio1 pf1 where > ID > > > > > >> 10 > > >>>>>>>>> order > > >>>>>>>>>>> by > > >>>>>>>>>>>>>> ID asc, pkid asc limit 5", "SELECT ID, description, > > >>>> createTime, > > >>>>>>>>> pkid > > >>>>>>>>>>>>> FROM > > >>>>>>>>>>>>>> /portfolio1 pf1 where ID > 10 and ID < 20 order by ID asc, > > >> pkid > > >>>>>>>> desc > > >>>>>>>>>>>>> limit > > >>>>>>>>>>>>>> 5 ", "SELECT ID, description, createTime, pkid FROM > > >>>> /portfolio1 > > >>>>>>>> pf1 > > >>>>>>>>>>>>> where > > >>>>>>>>>>>>>> ID > 10 and ID < 20 order by ID desc, pkid asc limit 5", > > >> "SELECT > > >>>>>>>>> ID, > > >>>>>>>>>>>>>> description, createTime, pkid FROM /portfolio1 pf1 where > ID > > >= > > >>>> 10 > > >>>>>>>> and > > >>>>>>>>>>> ID > > >>>>>>>>>>>>> <= > > >>>>>>>>>>>>>> 20 order by ID desc, pkid desc limit 5", "SELECT ID, > > >>>> description, > > >>>>>>>>>>>>>> createTime, pkid FROM /portfolio1 pf1 where ID >= 10 and > ID > > <= > > >>>> 20 > > >>>>>>>>> order > > >>>>>>>>>>>>> by > > >>>>>>>>>>>>>> ID asc, pkid asc limit 5", "SELECT ID, description, > > >>>> createTime, > > >>>>>>>>> pkid > > >>>>>>>>>>>>> FROM > > >>>>>>>>>>>>>> /portfolio1 pf1 where ID != 10 order by ID asc , pkid desc > > >> limit > > >>>>>>>> 10", > > >>>>>>>>>>>>>> "SELECT ID, description, createTime, pkid FROM > /portfolio1 > > >> pf1 > > >>>>>>>>> where > > >>>>>>>>>>> ID > > >>>>>>>>>>>>>> != 10 order by ID desc, pkid desc limit 10", > > >>>>>>>>>>>>>> + > > >>>>>>>>>>>>>> + }; > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> On Thu, Oct 13, 2016 at 9:51 AM, Jared Stewart < > > >>>>>>>> jstew...@pivotal.io> > > >>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>> The task is fully suppressible with -x spotlessCheck. > > Also, > > >> if > > >>>>>>>> you > > >>>>>>>>>>> have > > >>>>>>>>>>>>>>> any formatter errors you can automatically fix them with > > >>>> 'gradle > > >>>>>>>>>>>>>>> spotlessApply’. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> On Oct 13, 2016, at 9:40 AM, Kevin Duling < > > >> kdul...@pivotal.io > > >>>>> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>> If we made formatting a warning, then people would > > probably > > >>>>>>>> quickly > > >>>>>>>>>>>>>>> ignore > > >>>>>>>>>>>>>>>> it. > > >>>>>>>>>>>>>>>> If we made formatting an error, we need to be sure we > > don't > > >>>> get > > >>>>>>>> in > > >>>>>>>>> to > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>> situation where <editor of choice>'s formatter is not in > > >>>>>>>> agreement > > >>>>>>>>>>> with > > >>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>> build's checker. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> I can live with an additional 17 seconds as well. And > > >> Jared's > > >>>>>>>>>>> already > > >>>>>>>>>>>>>>>> reduced the build time locally by 50%. But I still want > > the > > >>>>>>>>> ability > > >>>>>>>>>>> to > > >>>>>>>>>>>>>>>> suppress the check similar to -x javadoc. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 9:58 PM, William Markito < > > >>>>>>>>>>> wmark...@pivotal.io> > > >>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> This sounds really good to me as well. +1 > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 4:13 PM, Jared Stewart < > > >>>>>>>>> jstew...@pivotal.io > > >>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>> This is running locally on my laptop. Since Spotless > is > > >>>> only > > >>>>>>>>> doing > > >>>>>>>>>>>>>>> code > > >>>>>>>>>>>>>>>>>> formatting and not any other static analysis, it > already > > >>>> has 0 > > >>>>>>>>>>>>> errors. > > >>>>>>>>>>>>>>>>>> (Other than, of course, formatting not consistent with > > the > > >>>>>>>>>>> template.) > > >>>>>>>>>>>>>>>>>>> On Oct 12, 2016, at 4:11 PM, Kenneth Howe < > > >>>> kh...@pivotal.io> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>> Agree with Mark, this has to work with 0 errors > before > > it > > >>>>>>>> would > > >>>>>>>>> be > > >>>>>>>>>>>>>>>>>> useful in precheckin. I think I could live with an > > >>>> additional > > >>>>>>>> 17 > > >>>>>>>>>>>>>>> seconds > > >>>>>>>>>>>>>>>>>> most of the time for running the spotlessCheck as > > >> suggested. > > >>>>>>>>>>>>>>>>>>> Jared, Is that 17 seconds running locally on your > > laptop > > >> or > > >>>>>>>> on a > > >>>>>>>>>>>>> more > > >>>>>>>>>>>>>>>>>> capable machine? > > >>>>>>>>>>>>>>>>>>> Ken > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>> On Oct 12, 2016, at 3:39 PM, Jared Stewart < > > >>>>>>>>> jstew...@pivotal.io> > > >>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>> If you want to try it out, I pushed a branch to my > > Geode > > >>>> repo > > >>>>>>>>>>> that > > >>>>>>>>>>>>>>>>>> contains this change: > > >>>>>>>>>>>>>>>>>>>> https://github.com/jaredjstewart/incubator-geode/ > > >>>>>>>>>>>>> tree/spotlessPlugin > > >>>>>>>>>>>>>>>>> < > > >>>>>>>>>>>>>>>>>> https://github.com/jaredjstewart/incubator-geode/ > > >>>>>>>>>>> tree/spotlessPlugin > > >>>>>>>>>>>>>>>>>>>>> On Oct 12, 2016, at 2:27 PM, Darrel Schneider < > > >>>>>>>>>>>>>>> dschnei...@pivotal.io > > >>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>> I like Dan's idea of catching formatting issues > > earlier > > >>>> but > > >>>>>>>> I > > >>>>>>>>>>>>> think > > >>>>>>>>>>>>>>>>>> adding > > >>>>>>>>>>>>>>>>>>>>> 5-10 minutes to "build" would be too much. > Currently > > >> when > > >>>>>>>> I'm > > >>>>>>>>>>>>> trying > > >>>>>>>>>>>>>>>>>> to do > > >>>>>>>>>>>>>>>>>>>>> a quick build I use -xjavadoc. I'd probably do the > > same > > >>>> for > > >>>>>>>>> this > > >>>>>>>>>>>>>>>>>> target if > > >>>>>>>>>>>>>>>>>>>>> it was part of build until I'm ready to do a > > >> precheckin. > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> Mark, wouldn't running the formatter on all our > java > > >>>> files > > >>>>>>>> and > > >>>>>>>>>>>>>>>>> checking > > >>>>>>>>>>>>>>>>>>>>> them in get these issues down to 0? > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 12:53 PM, Udo Kohlmeyer < > > >>>>>>>>>>>>>>>>> ukohlme...@pivotal.io > > >>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> +1 - adding checkstyle to precheckin. > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> If the developer uses the provided templates ( > > >> eclipse + > > >>>>>>>>>>>>> intellij) > > >>>>>>>>>>>>>>>>>> then > > >>>>>>>>>>>>>>>>>>>>>> most of the formatting issues should be handled > > before > > >>>>>>>>>>>>> precheckin. > > >>>>>>>>>>>>>>>>>> Also, if > > >>>>>>>>>>>>>>>>>>>>>> a developer has a questionable coding style, that > > >> should > > >>>>>>>>> lessen > > >>>>>>>>>>>>> as > > >>>>>>>>>>>>>>>>>> that > > >>>>>>>>>>>>>>>>>>>>>> developer will have resolve the issues before > being > > >>>> able to > > >>>>>>>>>>>>> commit. > > >>>>>>>>>>>>>>>>>>>>>> I also believe that this should not be an > > overbearing > > >>>> and > > >>>>>>>>>>>>> intrusive > > >>>>>>>>>>>>>>>>>>>>>> process. > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> --Udo > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> On 13/10/16 6:36 am, Mark Bretl wrote: > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> Dan, > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> There is some extra amount of time, 5-10 minutes > > >> extra > > >>>> for > > >>>>>>>>> the > > >>>>>>>>>>>>>>>>> entire > > >>>>>>>>>>>>>>>>>>>>>>> project (depending on your CPU). I think the real > > >>>> issue to > > >>>>>>>>>>>>> adding > > >>>>>>>>>>>>>>>>> it > > >>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>> precheckin target and have it be 'effective' is > it > > >>>> needs > > >>>>>>>> to > > >>>>>>>>>>> run > > >>>>>>>>>>>>>>>>>>>>>>> successfully, otherwise it would turn into noise > > most > > >>>> of > > >>>>>>>> the > > >>>>>>>>>>>>> time > > >>>>>>>>>>>>>>> I > > >>>>>>>>>>>>>>>>>> think. > > >>>>>>>>>>>>>>>>>>>>>>> We need to get the issues down to 0 or manage to > > set > > >> a > > >>>> new > > >>>>>>>>>>>>>>> baseline > > >>>>>>>>>>>>>>>>>> (not > > >>>>>>>>>>>>>>>>>>>>>>> the best idea), which is a lot of work, to make > it > > >> run > > >>>>>>>>>>>>>>>>> successfully. > > >>>>>>>>>>>>>>>>>> Right > > >>>>>>>>>>>>>>>>>>>>>>> now, if you run the target, it will fail every > time > > >>>> since > > >>>>>>>>>>> there > > >>>>>>>>>>>>>>>>>>>>>>> outstanding > > >>>>>>>>>>>>>>>>>>>>>>> issues in the code and very hard to tell what > > issues > > >>>> were > > >>>>>>>>>>>>>>>>> introduced. > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> --Mark > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 11:34 AM, Dan Smith < > > >>>>>>>>>>> dsm...@pivotal.io> > > >>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>> Seems like it should run as part of the build > > target. > > >>>> The > > >>>>>>>>> only > > >>>>>>>>>>>>>>>>>> reason to > > >>>>>>>>>>>>>>>>>>>>>>>> make it part of precheckin is if it takes a long > > >> time, > > >>>>>>>>>>>>> otherwise > > >>>>>>>>>>>>>>>>>> people > > >>>>>>>>>>>>>>>>>>>>>>>> should get fast feedback they need to change > their > > >>>> code > > >>>>>>>>>>> before > > >>>>>>>>>>>>>>>>> they > > >>>>>>>>>>>>>>>>>> push. > > >>>>>>>>>>>>>>>>>>>>>>>> -Dan > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 11:24 AM, Jared Stewart > < > > >>>>>>>>>>>>>>>>>> jstew...@pivotal.io> > > >>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> +1 to running during the precheckin target as > well > > >> as > > >>>>>>>>> Travis > > >>>>>>>>>>> CI > > >>>>>>>>>>>>>>>>>>>>>>>>> On Oct 12, 2016 11:20 AM, "Darrel Schneider" < > > >>>>>>>>>>>>>>>>>> dschnei...@pivotal.io> > > >>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> If Travis CI is only run on pull requests then > > that > > >>>> is > > >>>>>>>> not > > >>>>>>>>>>>>>>> enough > > >>>>>>>>>>>>>>>>>>>>>>>>> because > > >>>>>>>>>>>>>>>>>>>>>>>>> committers do not submit pull requests. Having > it > > >> run > > >>>>>>>>> during > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>> gradle > > >>>>>>>>>>>>>>>>>>>>>>>>>> build or precheckin target is also needed. In > > >>>> addition > > >>>>>>>> to > > >>>>>>>>>>>>> that > > >>>>>>>>>>>>>>> I > > >>>>>>>>>>>>>>>>>> also > > >>>>>>>>>>>>>>>>>>>>>>>>>> wanted PRs to be checked. > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 11:12 AM, Jared > Stewart > > < > > >>>>>>>>>>>>>>>>>> jstew...@pivotal.io> > > >>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> It would certainly be necessary to make sure > > that > > >>>> the > > >>>>>>>>> code > > >>>>>>>>>>>>>>> style > > >>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>>>>>> enforced is sensible, e.g. doe not use wildcard > > >>>> imports. > > >>>>>>>>> We > > >>>>>>>>>>>>>>>>> would > > >>>>>>>>>>>>>>>>>>>>>>>>>> also > > >>>>>>>>>>>>>>>>>>>>>>>>> want to make one large commit to format all > > >> existing > > >>>>>>>> code > > >>>>>>>>>>>>> before > > >>>>>>>>>>>>>>>>>>>>>>>>>> turning > > >>>>>>>>>>>>>>>>>>>>>>>>>> this on. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Mark - Thank you for the information about > the > > >>>>>>>> existing > > >>>>>>>>>>>>> setup. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Mark, Darrel, Kevin - Given all of your > > >> comments, I > > >>>>>>>>> think > > >>>>>>>>>>> it > > >>>>>>>>>>>>>>>>>> might > > >>>>>>>>>>>>>>>>>>>>>>>>>> make > > >>>>>>>>>>>>>>>>>>>>>>>>> more sense to add the flag to enable it in > Travis > > >> CI > > >>>>>>>>> rather > > >>>>>>>>>>>>> than > > >>>>>>>>>>>>>>>>> as > > >>>>>>>>>>>>>>>>>>>>>>>>>> part > > >>>>>>>>>>>>>>>>>>>>>>>>>> of the build. This way your build pass > > >> regardless > > >>>> of > > >>>>>>>>>>>>>>>>> whitespace, > > >>>>>>>>>>>>>>>>>>>>>>>>>> but > > >>>>>>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>> CI job would fail on PRs if they did not > adhere > > >> to > > >>>> the > > >>>>>>>>>>>>>>> standard > > >>>>>>>>>>>>>>>>>>>>>>>>>> formatting. > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Anthony - It doesn’t seem to me that turning > > this > > >>>> on > > >>>>>>>>> would > > >>>>>>>>>>>>>>> have > > >>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>> effect > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> of combining reformatting commits and logic > > >>>> changes. > > >>>>>>>>>>>>> Rather, > > >>>>>>>>>>>>>>>>>> since > > >>>>>>>>>>>>>>>>>>>>>>>>>> all > > >>>>>>>>>>>>>>>>>>>>>>>>> code would already be formatted, there would no > > >>>> longer > > >>>>>>>> be > > >>>>>>>>>>> any > > >>>>>>>>>>>>>>>>>>>>>>>>>> reformatting > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> commits except for single large commits when > > the > > >>>> code > > >>>>>>>>>>> style > > >>>>>>>>>>>>>>>>> file > > >>>>>>>>>>>>>>>>>> was > > >>>>>>>>>>>>>>>>>>>>>>>>>>> updated. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> On Oct 12, 2016, at 11:01 AM, Bruce > Schuchardt > > < > > >>>>>>>>>>>>>>>>>>>>>>>>>>> bschucha...@pivotal.io > > >>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> I like the idea of doing this but I don't > > think > > >>>>>>>>>>> Checkstyle > > >>>>>>>>>>>>>>>>>> should > > >>>>>>>>>>>>>>>>>>>>>>>>>>> be > > >>>>>>>>>>>>>>>>>>>>>>>>> enabled until all of the code is reformatted. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Also, last time I checked there was still a > > >>>> problem > > >>>>>>>>> with > > >>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>> IntelliJ > > >>>>>>>>>>>>>>>>>>>>>>>>> auto-format settings. It still used wildcard > > >>>> imports, > > >>>>>>>>>>> which I > > >>>>>>>>>>>>>>>>>>>>>>>>>> believe > > >>>>>>>>>>>>>>>>>>>>>>>>> we > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> don't allow. I've manually changed my > settings > > in > > >>>>>>>>>>>>> Editor->Code > > >>>>>>>>>>>>>>>>>>>>>>>>>> Style->Java > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> to "Use single class import" to correct that > > >>>>>>>> problem. I > > >>>>>>>>>>>>>>>>>> couldn't see > > >>>>>>>>>>>>>>>>>>>>>>>>>> how > > >>>>>>>>>>>>>>>>>>>>>>>>>> to get Gradle to do it. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Le 10/12/2016 à 10:28 AM, Anthony Baker a > > écrit > > >> : > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Source code with a consistent look-and-feel > > >>>> makes it > > >>>>>>>>>>>>> easier > > >>>>>>>>>>>>>>>>> for > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> people > > >>>>>>>>>>>>>>>>>>>>>>>>>> to join the project community and contribute. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Let’s continue to keep reformatting commits > > >>>> separate > > >>>>>>>>> from > > >>>>>>>>>>>>>>>>> logic > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> changes—otherwise it’s too hard to review. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> Anthony > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Oct 12, 2016, at 10:06 AM, Dan Smith < > > >>>>>>>>>>>>> dsm...@pivotal.io> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>> +1 > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This might be a good time to reformat the > > code > > >>>>>>>> since > > >>>>>>>>> I > > >>>>>>>>>>>>>>> don't > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> think > > >>>>>>>>>>>>>>>>>>>>>>>>> there > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> are too many long lived feature branches > > >>>> outstanding. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -Dan > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 10:00 AM, Jared > > >> Stewart > > >>>> < > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> jstew...@pivotal.io > > >>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> I would like to advocate for adding a > > >> Checkstyle < > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> http://checkstyle > > >>>>>>>>>>>>>>>>>>>>>>>>>> . > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> sourceforge.net/> or Spotless < > > >>>>>>>>>>> https://github.com/diffplug/ > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> spotless > > >>>>>>>>>>>>>>>>>>>>>>>>>> gradle task to our build process to ensure > that > > >> all > > >>>>>>>> code > > >>>>>>>>>>>>>>> checked > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in > > >>>>>>>>>>>>>>>>>>>>>>>>>> meets > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> the formatting standards described on the > > wiki < > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/ > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> confluence/display/GEODE/Code+Style+Guide> > > (and > > >>>> in > > >>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> intellij/eclipse > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> formatter xml files in our repository). This > > >> will > > >>>>>>>>>>> alleviate > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> difficulties > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> reviewing code when whitespace or formatting > > has > > >>>>>>>>> changed > > >>>>>>>>>>>>>>> since > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> all > > >>>>>>>>>>>>>>>>>>>>>>>>> code > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> checked in will already comply with > standards. > > >>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> -- > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> ~/William > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>>> > > >> > > >> > > > > >
