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