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