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

Reply via email to