+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