+1 to a) embracing https://google.github.io/styleguide/javaguide.html 100%,
b) adopting the google formatters for IntelliJ and Eclipse, c)using spotless


On Fri, Oct 14, 2016 at 6:56 AM, Jacob Barrett <jbarr...@pivotal.io> wrote:

> +1
>
> On Thu, Oct 13, 2016 at 10:04 AM Kevin Duling <kdul...@pivotal.io> wrote:
>
> > Given that, +1 from me!
> >
> > 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