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