Improve resiliency to auto-formatting in server #48450

pugnascotia · 2019-10-24T10:05:34Z

Make a number of changes so that code in the server directory is more resilient to automatic formatting. This covers:

Reformatting multiline JSON to embed whitespace in the strings
Move some comments around to they aren't auto-formatted to a strange
place. This also required moving some && and || operators from the
end-of-line to start-of-line`.
Turn of formatting of data tables in HyperLogLogPlusPlus. This also
required a checkstyle suppression.
Exclude HyperLogLogPlusPlus from being formatted. It would format OK if the rules for wrapping array contents were set to "wrap when necessary", but that would be worse for the rest of the codebase. Instead, I've applied to formatter to the rest of the file, for consistency, but left the data tables alone.
Add helper method reformatJson(), to strip whitespace from a JSON
document using XContent methods. This is sometimes necessary where
a test is comparing some machine-generated JSON with an expected
value.

Make a number of changes so that code in the `server` directory is more resilient to automatic formatting. This covers: * Reformatting multiline JSON to embed whitespace in the strings * Move some comments around to they aren't auto-formatted to a strange place. This also required moving some `&&` and `||` operators from the end-of-line to start-of-line`. * Turn of formatting of data tables in `HyperLogLogPlusPlus`. This also required a checkstyle suppression. * Add helper method `reformatJson()`, to strip whitespace from a JSON document using XContent methods. This is sometimes necessary where a test is comparing some machine-generated JSON with an expected value.

These have moved to PR elastic#48553.

pugnascotia · 2019-10-26T14:14:58Z

I've moved the JSON-related changes in this PR to #48553 in order to reduce the number of changes here.

…server

elasticmachine · 2019-10-27T16:10:14Z

Pinging @elastic/es-core-infra (:Core/Infra/Core)

alpar-t · 2019-10-29T06:38:57Z

buildSrc/src/main/resources/checkstyle_suppressions.xml

@@ -46,6 +46,10 @@
  <suppress files="client[/\\]rest-high-level[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]client[/\\]documentation[/\\]WatcherDocumentationIT.java" id="SnippetLength" />
  <suppress files="modules[/\\]reindex[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]client[/\\]documentation[/\\]ReindexDocumentationIT.java" id="SnippetLength" />

+  <!-- These use `// tag::` and `// end::` just to turn off auto-formatting, but
+       as they have long lines the trigger the SnippetLength check -->
+  <suppress files="server[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]search[/\\]aggregations[/\\]metrics[/\\]HyperLogLogPlusPlus.java" id="SnippetLength" />


I think that's an important check for the documentation I don't think we should be turning it off

HyperLogLogPlusPlus.java doesn't have any existing doc snippets, so right now it's safe to turn off the check for this file only. I suppose I could have reformatted the tagged sections to meet the width limit, but I didn't want to mess around the file in that way. I could add some comments in the file to explain the situation about formatting in greater detail? What do you think?

This seems like a hack. I don't think we should mark something as a doc snippet if it isn't intended to be used in the docs. What about that file does not auto format well? Is there anything we can change about the format rules to make it better?

I think "hack" is a fair assessment. However, the Eclipse formatter doesn't have support for recognising tables of data (it's the sole feature from the Google formatter than I miss - at least, I'm sure I read that somewhere), so the arrays of data get spread over many, many, many lines. Now, you could argue that this is irrelevant - semantically it makes no difference - but I didn't feel comfortable making that call.

You also can't define multiple tags for disabling and enabling the formatter, and we need to be able to use them for legitimate docs snippets.

The other option is to alter the source to build up the data arrays in pieces and then combine them. This is altering the source to please the compiler, which isn't great. Or define the data in text files or some such, and parse them, which is also a poor option.

...actually, thinking about it again, I suppose we could change the formatter config to only wrap where necessary in array initializers. That would be perfect for HyperLogLogPlusPlus.java, but probably worsen readability for other arrays in the codebase. We'd then be configuring formatter for the exceptional cases.

@rjernst I've taken a new approach to HyperLogLogPlusPlus, by excluding it from being formatted automatically. I've applied to formatter to the rest of the file, for consistency, but left the data tables alone.

alpar-t

LGTM in general do have once concern about check style

…server

pugnascotia · 2019-10-29T11:59:45Z

@elasticmachine run elasticsearch-ci/1

pugnascotia · 2019-10-31T10:49:25Z

Still looking for an approval on this one.

…server

I had added doc snippet tags to HyperLogLogPlusPlus in order to stop the data tables being mangled when we come to automatically format the codebase. However, since the lines of code were longer than 76 characters, I also added a Checkstyle suppression. That's a slippery slope, however, so instead I've reformatted the long lines and removed the suppression.

Don't automatically reformat HyperLogLogPlusPlus. It's the cleanest way to avoid mangling the data tables it contains, without also hacking about with doc tag snippets, manual reformatting or checkstyle exclusions.

pugnascotia · 2019-11-05T11:07:30Z

@elasticmachine run elasticsearch-ci/2

pugnascotia · 2019-11-08T13:55:28Z

Any more review comments on this?

alpar-t · 2019-11-11T06:49:17Z

build.gradle

@@ -114,6 +114,10 @@ subprojects {

      spotless {
        java {
+          if (project.path.equals(':server')) {


Would be better to just add this to server/build.gradle

I didn't think that would work, but turns out it does, which is much tidier, thank you. Unfortunately, I then had to wrap the config in a try/catch because unless a project opts-in to formatting, the spotless() method doesn't exist. I can removed the extra cruft once we enable formatting.

alpar-t

I added a minor comment, no need for another review.

…server

Make a number of changes so that code in the `server` directory is more resilient to automatic formatting. This covers: * Reformatting multiline JSON to embed whitespace in the strings * Move some comments around to they aren't auto-formatted to a strange place. This also required moving some `&&` and `||` operators from the end-of-line to start-of-line`. * Add helper method `reformatJson()`, to strip whitespace from a JSON document using XContent methods. This is sometimes necessary where a test is comparing some machine-generated JSON with an expected value. Also, `HyperLogLogPlusPlus.java` is now excluded from formatting because it contains large data tables that don't reformat well with the current settings, and changing the settings would be worse for the rest of the codebase.

Backport of #48450. Make a number of changes so that code in the `server` directory is more resilient to automatic formatting. This covers: * Reformatting multiline JSON to embed whitespace in the strings * Move some comments around to they aren't auto-formatted to a strange place. This also required moving some `&&` and `||` operators from the end-of-line to start-of-line`. * Add helper method `reformatJson()`, to strip whitespace from a JSON document using XContent methods. This is sometimes necessary where a test is comparing some machine-generated JSON with an expected value. Also, `HyperLogLogPlusPlus.java` is now excluded from formatting because it contains large data tables that don't reformat well with the current settings, and changing the settings would be worse for the rest of the codebase.

pugnascotia added v8.0.0 v7.6.0 labels Oct 24, 2019

pugnascotia requested a review from rjernst October 24, 2019 10:05

pugnascotia mentioned this pull request Oct 24, 2019

Auto-formatting tweaks #47299

Closed

Test fixes

45443dc

pugnascotia mentioned this pull request Oct 26, 2019

Improve resiliency to formatting JSON in server #48553

Merged

Revert JSON changes

3a3df7a

These have moved to PR elastic#48553.

Merge remote-tracking branch 'upstream/master' into formatting-fixes-…

b121bee

…server

pugnascotia added the :Core/Infra/Core Core issues without another label label Oct 27, 2019

alpar-t reviewed Oct 29, 2019

View reviewed changes

pugnascotia added 2 commits October 29, 2019 10:45

Add explanation about doc snippets to HyperLogLogPlusPlus

d964c81

Merge remote-tracking branch 'upstream/master' into formatting-fixes-…

e382996

…server

pugnascotia added 3 commits November 5, 2019 09:37

Merge remote-tracking branch 'upstream/master' into formatting-fixes-…

4890e9b

…server

Exclude HyperLogLogPlusPlus from formatting

6c807a1

Don't automatically reformat HyperLogLogPlusPlus. It's the cleanest way to avoid mangling the data tables it contains, without also hacking about with doc tag snippets, manual reformatting or checkstyle exclusions.

alpar-t reviewed Nov 11, 2019

View reviewed changes

alpar-t approved these changes Nov 11, 2019

View reviewed changes

pugnascotia added 3 commits November 11, 2019 10:40

Move :server spotless exclusions to subproject

c2ad0e7

Merge remote-tracking branch 'upstream/master' into formatting-fixes-…

3b55a9c

…server

Guard against spotless() not existing in :server

c102df1

pugnascotia merged commit 47b4925 into elastic:master Nov 11, 2019

pugnascotia deleted the formatting-fixes-server branch November 11, 2019 11:45

pugnascotia mentioned this pull request Nov 11, 2019

Improve resiliency to auto-formatting in server #48940

Merged

$@polyfractal$ polyfractal added the >non-issue label Jan 15, 2020

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve resiliency to auto-formatting in server #48450

Improve resiliency to auto-formatting in server #48450

pugnascotia commented Oct 24, 2019 •

edited

Loading

pugnascotia commented Oct 26, 2019

elasticmachine commented Oct 27, 2019

alpar-t Oct 29, 2019

pugnascotia Oct 29, 2019

rjernst Oct 29, 2019

pugnascotia Oct 31, 2019

pugnascotia Nov 5, 2019

alpar-t left a comment

pugnascotia commented Oct 29, 2019

pugnascotia commented Oct 31, 2019

pugnascotia commented Nov 5, 2019

pugnascotia commented Nov 8, 2019

alpar-t Nov 11, 2019

pugnascotia Nov 11, 2019

alpar-t left a comment

Improve resiliency to auto-formatting in server #48450

Improve resiliency to auto-formatting in server #48450

Conversation

pugnascotia commented Oct 24, 2019 • edited Loading

pugnascotia commented Oct 26, 2019

elasticmachine commented Oct 27, 2019

alpar-t Oct 29, 2019

Choose a reason for hiding this comment

pugnascotia Oct 29, 2019

Choose a reason for hiding this comment

rjernst Oct 29, 2019

Choose a reason for hiding this comment

pugnascotia Oct 31, 2019

Choose a reason for hiding this comment

pugnascotia Nov 5, 2019

Choose a reason for hiding this comment

alpar-t left a comment

Choose a reason for hiding this comment

pugnascotia commented Oct 29, 2019

pugnascotia commented Oct 31, 2019

pugnascotia commented Nov 5, 2019

pugnascotia commented Nov 8, 2019

alpar-t Nov 11, 2019

Choose a reason for hiding this comment

pugnascotia Nov 11, 2019

Choose a reason for hiding this comment

alpar-t left a comment

Choose a reason for hiding this comment

pugnascotia commented Oct 24, 2019 •

edited

Loading