Hi there Patrick,
Given that you have almost being the token Spotless-trainer, I was just
wondering if we have ever updated the style guides (eclipse + intellij)
with the latest version from google?
https://github.com/google/styleguide
Whilst I am not across ANY of the changes or differences in the two
"style guides", it would be interesting to see if the two "guides" have
converged enough for us to use again.
Does it make sense that we rather state that we have based our style
guide on the Google guides, but have diverged so far that we cannot go back?
Do we have any idea as to why spotless would be flagging this as an
issue now? It is not like we have just recently introduced this animal
called spotless.
@all_devs
Whilst it has helped us maintain some sanity when it came to style, I
also feel that it has overstayed its welcome on many occasions. Caring
about spaces in comments is just a matter preference and does not carry
any that weight when it comes to code. I believe if we try and follow
the following guide, https://google.github.io/styleguide/javaguide.html,
we could potentially have spotless be the monitor for code conformity,
rather than the Skynet it has become.
--Udo
On 8/22/18 12:28, Patrick Rhomberg wrote:
I totally understand your frustrations, Kirk. It is disheartening to stub
your toe on someone else's rough corners.
This email became more verbose than I intended, which is par for the course
for me, but still... tl;dr: you can change whitespace rules in the
etc/eclipse*.xml. Feel free to go relax those rules, if you can figure out
which exactly which constants Eclipse wants set to what and where.
Personally, I don't have a horse in this race on what the whitespace rules
should be. I just think that we should have a standard that is enforced.
-----
Just to clarify some possible misconceptions:
* We don't actually use google-java-format utility. Spotless is provided
by DiffPlug and accepts an arbitrary Eclipse format file.
* We _do_ use a format spec that was initially based off the Google Java
format style guide. It can be found at the now-suboptimally-named
<geode>/etc/eclipse-java-google-style.xml.
* (Of course, problems in the google-java-format utility may share a root
cause of in that Eclipse format xml, so that google-java-format issue may
indeed be what we were hitting.)
* The Eclipse format xml should contain just about everything that deals
with whitespacing in our style.
* Everything else that spotless cares about is contained in
<geode>/gradle/spotless.gradle
* Most of our not-whitespace-related spotless constraints came from
observing repeated violations (read: "petty annoyances") throughout our own
codebase. Still, with few exceptions, you really shouldn't be hitting any
non-whitespace spotless issues if you are adhering to standard practices.
Our current spotless rules are as follows:
====
Best practices rules:
* Remove your unused imports.
* Remove your commented-out imports
* Do not use wildcard imports
* Remove empty Javadoc stubs (e.g., "@param myParam" with no description).
Better would be to write descriptive javadocs.
* Empty javadoc or block comments.
====
Whitespace rules:
* The litany of things defined in the Eclipse format file. (For instance,
if there is a setting for spaces after punctuation in comments, it would
live here somewhere.)
* Lines should not end with whitespace before the newline.
* A file should end with a newline.
====
Community standards and for consistency rules:
* We have an opinion on import order (Historically, there was a
reorder-war between Eclipse and IntelliJ users that disagreed on where
javax. should go. This was standardized to reduce diff noise in pull
requests.)
* We have an opinion on modifier word order. (This appears to be more of a
Java-community standard. Not a "best practice" per se, but helps with
readability.)
====
Known issue:
* There is some disagreement between the IntelliJ's style file in etc/ and
the Eclipse style file in the same directory.
* As a corollary and because Spotless uses the Eclipse style file, running
AutoFormat in IntelliJ can create formatting that is considered a violation
of spotless.
And that, everyone, is pretty much everything that I know about Spotless.
Imagination is Change.
~Patrick
On Wed, Aug 22, 2018 at 9:55 AM, Anthony Baker <aba...@pivotal.io> wrote:
Thanks Kirk! FWIW, I’m also annoyed with the overzealous spotless
constraints.
Anthony
On Aug 22, 2018, at 9:28 AM, Kirk Lund <kl...@apache.org> wrote:
Sorry to waste everyone’s time with something so trivial. I tried hard to
ensure my instructions for Setting Up IntelliJ were correct and
error-free,
so this formatter issue was a big disappointment to me because of that.
The
instructions have been updated.
On Tue, Aug 21, 2018 at 3:57 PM, Kirk Lund <kl...@apache.org> wrote:
Looks like it's a feature: https://github.com/google/
google-java-format/
issues/62
Is it too late to down-vote our use of google-java-format?
On Tue, Aug 21, 2018 at 3:43 PM, Kirk Lund <kl...@apache.org> wrote:
I suppose I was using that older format of the Apache license header
and
then using spotlessApply 100% before running spotlessCheck which was
reformatting the license header. So even though I was using the older
one,
I never ran into the problem until today.
So maybe nothing changed?
But, I still think it's ridiculous that we have spotless configured to
disallow a double-space after sentence terminator.
On Tue, Aug 21, 2018 at 3:39 PM, Kirk Lund <kl...@apache.org> wrote:
I know it's not a bug in spotless. I think we now have the settings a
bit too strict.
As of 2-3 weeks ago, I was able to follow the "Setting up IntelliJ"
process that I documented at https://github.com/gemfire/gemfire
(search
down for "Setting up IntelliJ") without spotless failing. See the
format of the Apache license header that's pasted into that readme?
It has
the extra spaces, including 2 spaces between sentences.
2-3 weeks ago, this was working fine. Now it fails spotless, so
something changed. Maybe the version of spotless that we're using in
gradle? Or a gradle spotless plugin version changed?
At best, it's laughable that our spotless format now complains about
correct English syntax in comments and javadocs. At worst, it's
evidence
that our use of spotless is... "a bit too strict" which in my opinion
should be fixed.
Can you please look into what changed? I haven't had much luck finding
it yet but I assure you that something did change.
On Tue, Aug 21, 2018 at 2:26 PM, Patrick Rhomberg <
prhomb...@pivotal.io>
wrote:
The only addition with respect to spotless on the 10th was to add the
`devBuild` target (which runs `spotlessApply`) and to require that
`spotlessApply` would run before `compileJava`, if both were to run
in a
given build command.
Looking at the PR against which these failed, it looks like it might
be
some disagreement between your IDE's desired format and spotless's.
Notably, the new test file header is thinner and has more space
padding. I
hadn't thought spotless cared about comment blocks, but looking now,
it
does look like we're consistent everywhere else (within the Java code
that
spotless targets) on how that header is formatted.
So, you know... It's a feature, not a bug? And we should investigate
the
discrepancies between the format files in <geode>/etc, that is, the
Eclipse
file spotless uses and the IntelliJ file that is meant to emulate it.
On Tue, Aug 21, 2018 at 9:48 AM, Kirk Lund <kl...@apache.org> wrote:
This appears to be caused by changes made to the build around August
10?
On Tue, Aug 21, 2018 at 9:38 AM, Kirk Lund <kl...@apache.org>
wrote:
Why is spotless now complaining about correct English? By correct
English,
I mean having 2 spaces between sentences in javadoc or comments (in
this
case it's the Apache license header):
-·*·the·License.··You·may·obtain·a·copy·of·the·License·at
+·*·the·License.·You·may·obtain·a·copy·of·the·License·at
Execution failed for task ':geode-core:spotlessJava'.
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1903
The following files had format violations:
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1904
geode-core/src/main/java/org/apache/geode/internal/cache/
RegionNameValidation.java
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1905
@@ -1,12 +1,12 @@
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1906
/*
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1907
·*·Licensed·to·the·Apache·Software·Foundation·(ASF)·
under·one·or·more
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1908
-·*·contributor·license·agreements.··See·the·NOTICE·
file·distributed·with
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1909
+·*·contributor·license·agreements.·See·the·NOTICE·
file·distributed·with
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1910
·*·this·work·for·additional·information·regarding·
copyright·ownership.
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1911
·*·The·ASF·licenses·this·file·to·You·under·the·Apache·
License,·Version·2.0
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1912
·*·(the·"License");·you·may·not·use·this·file·except·in·
compliance·with
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1913
-·*·the·License.··You·may·obtain·a·copy·of·the·License·at
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1914
+·*·the·License.·You·may·obtain·a·copy·of·the·License·at
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1915
·*
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1916
-·*······http://www.apache.org/licenses/LICENSE-2.0
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1917
+·*·http://www.apache.org/licenses/LICENSE-2.0
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1918
·*
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1919
·*·Unless·required·by·applicable·law·or·agreed·to·
in·writing,·software
<https://concourse.apachegeode-ci.info/builds/19648#L5b60a2a0:1920
·*·distributed·under·the·License·is·distributed·on·an·"
AS·IS"·BASIS,
