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