+1 - Nice work. Thx
On 24/10/16 10:40 am, Dan Smith wrote:
Doing a spotlessApply on my feature branch before rebasing didn't help
bring down the number of conflicts.
I came up with this sequence of steps to rebase a feature branch on develop
that avoids the need to manually resolve conflicts with the formatting
changes. The trick here is to pick up *just* the formatting changes in one
of the steps, and then reject any formatting changes that conflict with my
changes.
#Rebase onto the commit before the spotless change. Resolve conflicts if any
git rebase 56917a26a8916b83f0cec6e85285b5040ff66ee6
#Rebase onto the spotless change, automatically throwing away the
formatting changes if they conflict.
git rebase -Xtheirs c2319bb7a6201d5ae82ecb0fe23a1e3b8072c2e1
#Rebase onto the rest of develop. Resolve conflicts if any.
git rebase origin/develop
#Apply formatting
./gradlew geode-core:spotlessApply
-Dan
On Fri, Oct 21, 2016 at 4:34 PM, Jared Stewart <jstew...@pivotal.io> wrote:
Try this commit hash instead: d0175ec5aa8acf1b34ece3183fe03e9874450cbb
(from feature/spotlessPlugin).
On Oct 21, 2016, at 4:16 PM, Kirk Lund <kl...@pivotal.io> wrote:
FYI, feeb5c98402881156b34e222c58ce15c71a4fca7 doesn't exist in the
Apache
git repo.
Is there a way to reformat a branch and then rebase on develop to
minimize
conflicts?
-Kirk
On Fri, Oct 21, 2016 at 1:36 PM, Jared Stewart <jstew...@pivotal.io>
wrote:
Fantastic, thanks for merging this in Mark. For anyone with outstanding
work on branches made before this change, your life may be made easier
by
cherry-picking feeb5c98402881156b34e222c58ce15c71a4fca7 (which added
Spotless) into your branch and then running ‘gradlew spotlessApply’ on
it
before attempting to merge into develop.
— Jared
On Oct 21, 2016, at 1:33 PM, Mark Bretl <asf.mbr...@gmail.com> wrote:
Thanks Jared for the suggestion of Spotless and follow-up work.
This is now completed and checked into develop. As this does touch many
files, be prepared the next time you pull.
--Mark
On Fri, Oct 21, 2016 at 1:21 PM, Jared Stewart <jstew...@pivotal.io>
wrote:
Done! :)
- Jared
On Oct 21, 2016, at 12:27 PM, Mark Bretl <asf.mbr...@gmail.com>
wrote:
One more time! :)
Conflicting files
geode-core/src/test/java/org/apache/geode/disttx/
PRDistTXDUnitTest.java
geode-core/src/test/java/org/apache/geode/disttx/
PRDistTXWithVersionsDUnitTest.java
geode-core/src/test/java/org/apache/geode/internal/cache/execute/
PRTransactionDUnitTest.java
--Mark
On Fri, Oct 21, 2016 at 12:08 PM, Jared Stewart <jstew...@pivotal.io
wrote:
I just pulled and rebased onto develop, and force pushed into the
existing
pull request. It should be clean to merge in now.
Thanks,
Jared
On Oct 21, 2016, at 11:57 AM, Mark Bretl <asf.mbr...@gmail.com>
wrote:
I believe there is enough consensus here to check this into
develop.
Jared, due to recent checkins into develop, can you update the pull
request
one more time? Trying to make this as clean as possible. I will
check
into
develop after the update, unless someone else gets to it first.
All, can we hold checkins on develop until the new formatter is
applied?
Thanks,
--Mark
On Fri, Oct 21, 2016 at 9:03 AM, Kenneth Howe <kh...@pivotal.io>
wrote:
+1
On Oct 21, 2016, at 8:27 AM, Bruce Schuchardt <
bschucha...@pivotal.io>
wrote:
+1
Le 10/20/2016 à 5:13 PM, Udo Kohlmeyer a écrit :
+1
On 20/10/16 4:56 pm, Mark Bretl wrote:
+1 as well...
- Pulled changes
- Executed './gradlew clean build' and was successful.
- Modified a couple of random files to test
- Ran './gradlew clean build' again and failed expectedly
- Ran './gradlew spotlessApply', task was successful
- Ran './gradlew clean build' and succeeded
Great addition! As long as others are good with the formatter,
then I
am
good.
--Mark
On Thu, Oct 20, 2016 at 3:40 PM, Kirk Lund <kl...@apache.org>
wrote:
+1 I just added my approval to the PR (and again here)
On Thu, Oct 20, 2016 at 3:25 PM, Jared Stewart <
jstew...@pivotal.io
wrote:
I have opened a pull request here <
https://github.com/apache/
incubator-geode/pull/268> to enable the Spotless plugin and
to
switch to
the Google Java Style formatter templates.
On Oct 18, 2016, at 4:32 PM, Kirk Lund <kl...@pivotal.io>
wrote:
For reference TRAC #38741 was a bug with the summary
"EOFException
during
deserialize on client update with copy-on-read=true"
-Kirk
On Tue, Oct 18, 2016 at 4:27 PM, Jared Stewart <
jstew...@pivotal.io
wrote:
To give everyone an update, using the Google Java Style
eclipse
template
there is an issue spotlessCheck where fails for
geode-core/src/test/java/org/apache/geode/cache30/
Bug38741DUnitTest.java
even if you run it directly after spotlessApply. This needs
to
be
investigated and fixed before I can open a pull request to
enable
spotless.
On Oct 14, 2016, at 4:57 PM, Dan Smith <dsm...@pivotal.io
wrote:
+1 - The formatting looks better now.
-Dan
On Thu, Oct 13, 2016 at 11:06 AM, Jared Stewart <
jstew...@pivotal.io
wrote:
I agree that the formatter needs fixing up. Our wiki <
https://cwiki.apache.org/confluence/display/GEODE/Code+
Style+Guide>
says
that we follow the Google Java Style guide, but that is
not
actually
what’s
in our formatter templates. I pushed a new branch <
https://github.com/
jaredjstewart/incubator-geode/tree/
spotlessPluginGoogleStyle>
that
points
spotless at the actual Google Java Style template, and I
think
it
makes
both of the examples you found look better.
On Oct 13, 2016, at 10:11 AM, Dan Smith <
dsm...@pivotal.io>
wrote:
+1 for adding this to ./gradlew build
But I think we might want to fix up the formatter a bit
before
reformatting
the code. I tried running spotlessApply, and it did some
unfortunate
reformatting of code to make it less readable.
One problem is with method chaining. We have a few
different
factory
APIs
that encourage method chaining, and it put all the
method
calls
on
a
single
line. For example here's one change:
- ClientCacheFactory ccf = new
ClientCacheFactory()
-
.addPoolServer(NetworkUtils.getServerHostName(server1.
getHost()),
port)
- .set(SECURITY_CLIENT_AUTH_INIT,
UserPasswordAuthInit.class.getName() + ".create")
- .set(SECURITY_PREFIX+"username", "root")
- .set(SECURITY_PREFIX+"password", "root");
+ ClientCacheFactory ccf = new
ClientCacheFactory().addPoolServer(NetworkUtils.
getServerHostName(server1.getHost()),
port).set(SECURITY_CLIENT_AUTH_INIT,
UserPasswordAuthInit.class.
getName()
+
".create").set(SECURITY_PREFIX + "username",
"root").set(SECURITY_PREFIX
+
"password", "root");
I see a similar problem where it put array
initialization
all
on a
single
line:
+ public void testMultiColOrderByWithIndexRe
sultWithProjection()
throws
Exception {
String queries[] = {
// Test case No. IUMR021
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID desc, pkid desc ",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID asc, pkid asc ",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 and ID < 20 order by ID asc, pkid asc ",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 and ID < 20 order by ID desc , pkid desc",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID >= 10 and ID <= 20 order by ID desc, pkid asc
",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID >= 10 and ID <= 20 order by ID asc, pkid desc",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID != 10 order by ID asc , pkid desc",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID != 10 order by ID desc, pkid asc ",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID desc, pkid desc limit 5",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID asc, pkid asc limit 5",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 and ID < 20 order by ID asc, pkid desc
limit 5
",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 and ID < 20 order by ID desc, pkid asc
limit
5",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID >= 10 and ID <= 20 order by ID desc, pkid desc
limit
5",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID >= 10 and ID <= 20 order by ID asc, pkid asc
limit
5",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID != 10 order by ID asc , pkid desc limit 10",
- "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID != 10 order by ID desc, pkid desc limit 10",
-
- };
+ "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID desc, pkid desc ", "SELECT
ID,
description,
createTime, pkid FROM /portfolio1 pf1 where ID > 10
order
by
ID
asc,
pkid
asc ", "SELECT ID, description, createTime, pkid FROM
/portfolio1
pf1
where ID > 10 and ID < 20 order by ID asc, pkid asc ",
"SELECT
ID,
description, createTime, pkid FROM /portfolio1 pf1 where
ID
10
and
ID <
20 order by ID desc , pkid desc", "SELECT ID,
description,
createTime,
pkid FROM /portfolio1 pf1 where ID >= 10 and ID <= 20
order
by
ID
desc,
pkid asc ", "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1 where ID >= 10 and ID <= 20 order by ID asc, pkid
desc",
"SELECT
ID,
description, createTime, pkid FROM /portfolio1 pf1 where
ID
!=
10
order
by
ID asc , pkid desc", "SELECT ID, description,
createTime,
pkid
FROM
/portfolio1 pf1 where ID != 10 order by ID desc, pkid
asc
",
+ "SELECT ID, description, createTime, pkid
FROM
/portfolio1
pf1
where ID > 10 order by ID desc, pkid desc limit 5",
"SELECT
ID,
description, createTime, pkid FROM /portfolio1 pf1 where
ID
10
order
by
ID asc, pkid asc limit 5", "SELECT ID, description,
createTime,
pkid
FROM
/portfolio1 pf1 where ID > 10 and ID < 20 order by ID
asc,
pkid
desc
limit
5 ", "SELECT ID, description, createTime, pkid FROM
/portfolio1
pf1
where
ID > 10 and ID < 20 order by ID desc, pkid asc limit 5",
"SELECT
ID,
description, createTime, pkid FROM /portfolio1 pf1 where
ID
=
10
and
ID
<=
20 order by ID desc, pkid desc limit 5", "SELECT ID,
description,
createTime, pkid FROM /portfolio1 pf1 where ID >= 10 and
ID
<=
20
order
by
ID asc, pkid asc limit 5", "SELECT ID, description,
createTime,
pkid
FROM
/portfolio1 pf1 where ID != 10 order by ID asc , pkid
desc
limit
10",
"SELECT ID, description, createTime, pkid FROM
/portfolio1
pf1
where
ID
!= 10 order by ID desc, pkid desc limit 10",
+
+ };
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
