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 testMultiColOrderByWithIndexResultWithProjection() 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 >>>>>> >>>> >>>> >> >>
