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