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

Reply via email to