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

Reply via email to