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