Formatting escape hatch (#81806)

Thanks to https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437, we've run into a situation where Spotless is incorrectly formatting a particular piece of syntax (due the underlying Eclipse bug). We were able to turn off formatting of this syntax using `// @Formatter:off` and `// @Formatter:on`, but there was a further problem. We configure IntelliJ to use the Eclipse formatter plugin, but this doesn't respect the `@formatter` tags since these are set at the Spotless level, not the Eclipse formatter level. Note that these tags aren't set in the Eclipse formatter config, because there we use `// tag::` and `// end::` in order to avoid reformatting docs snippets, which have a much narrower line width. What a mess. So, to get around all this, drop the `@formatter` tags and tweak our custom `SnippetLengthCheck` Checkstyle rule so that `// tag:noformat` regions are not subject to the narrower line length check, but are still exempt from formatting.
elastic · Dec 16, 2021 · 358cd80 · 358cd80
1 parent d5222bd
commit 358cd80
Show file tree

Hide file tree

Showing 11 changed files with 25 additions and 29 deletions.
diff --git a/.editorconfig b/.editorconfig
@@ -10,10 +10,6 @@ trim_trailing_whitespace = true
 insert_final_newline = true
 indent_style = space
 
-ij_formatter_off_tag = @formatter:off
-ij_formatter_on_tag = @formatter:on
-ij_formatter_tags_enabled = false
-
 [*.gradle]
 ij_continuation_indent_size = 2
 indent_size = 2

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -305,7 +305,7 @@ Please follow these formatting guidelines:
 * Wildcard imports (`import foo.bar.baz.*`) are forbidden and will cause
   the build to fail.
 * If *absolutely* necessary, you can disable formatting for regions of code
-  with the `// @formatter:off` and `// @formatter:on` directives, but
+  with the `// tag::noformat` and `// end::noformat` directives, but
   only do this where the benefit clearly outweighs the decrease in formatting
   consistency.
 * Note that Javadoc and block comments i.e. `/* ... */` are not formatted,

diff --git a/...ntions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java b/...ntions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java
@@ -22,9 +22,12 @@
 /**
  * Checks the snippets included in the docs aren't too wide to fit on
  * the page.
+ * <p>
+ * Regions contained in the special <code>noformat</code> tag are exempt from the length
+ * check. This region is also exempt from automatic formatting.
  */
 public class SnippetLengthCheck extends AbstractFileSetCheck {
-    private static final Pattern START = Pattern.compile("^( *)//\\s*tag::(.+?)\\s*$", Pattern.MULTILINE);
+    private static final Pattern START = Pattern.compile("^( *)//\\s*tag::(?!noformat)(.+?)\\s*$", Pattern.MULTILINE);
     private int max;
 
     /**

diff --git a/...va/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java b/...va/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java
@@ -60,9 +60,6 @@ public void apply(Project project) {
 
                 java.target("src/**/*.java");
 
-                // Use `@formatter:off` and `@formatter:on` to toggle formatting - ONLY IF STRICTLY NECESSARY
-                java.toggleOffOn("@formatter:off", "@formatter:on");
-
                 java.removeUnusedImports();
 
                 // We enforce a standard order for imports

diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/graph/Vertex.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/graph/Vertex.java
@@ -154,7 +154,7 @@ public void setWeight(final double weight) {
         this.weight = weight;
     }
 
-    // @formatter:off
+    // tag::noformat
     /**
      * If the {@link GraphExploreRequest#useSignificance(boolean)} is true (the default)
      * this statistic is available.
@@ -163,12 +163,12 @@ public void setWeight(final double weight) {
      * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html"
      * >the significant_terms aggregation</a>)
      */
-    // @formatter:on
+    // end::noformat
     public long getBg() {
         return bg;
     }
 
-    // @formatter:off
+    // tag::noformat
     /**
      * If the {@link GraphExploreRequest#useSignificance(boolean)} is true (the default)
      * this statistic is available.
@@ -178,7 +178,7 @@ public long getBg() {
      * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html"
      * >the significant_terms aggregation</a>)
      */
-    // @formatter:on
+    // end::noformat
     public long getFg() {
         return fg;
     }

diff --git a/client/rest/src/main/java/org/elasticsearch/client/Response.java b/client/rest/src/main/java/org/elasticsearch/client/Response.java
@@ -120,7 +120,7 @@ public HttpEntity getEntity() {
      * Length of RFC 1123 format (with quotes and leading space), used in
      * matchWarningHeaderPatternByPrefix(String).
      */
-    // @formatter:off
+    // tag::noformat
     private static final int WARNING_HEADER_DATE_LENGTH = 0
             + 1
             + 1
@@ -131,7 +131,7 @@ public HttpEntity getEntity() {
             + 2 + 1 + 2 + 1 + 2 + 1
             + 3
             + 1;
-    // @formatter:on
+    // end::noformat
 
     /**
      * Tests if a string matches the RFC 7234 specification for warning headers.

diff --git a/.../elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java b/.../elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java
@@ -439,7 +439,7 @@ public void testFieldIsntWrittenOutTwice() throws Exception {
         // you need to add an additional index with no fields in order to trigger this (or potentially a shard)
         // so that there is an UnmappedTerms in the list to reduce.
         createIndex("foo_1");
-        // @formatter:off
+        // tag::noformat
         XContentBuilder builder = jsonBuilder().startObject()
             .startObject("properties")
               .startObject("@timestamp")
@@ -463,17 +463,17 @@ public void testFieldIsntWrittenOutTwice() throws Exception {
               .endObject()
             .endObject()
           .endObject();
-        // @formatter:on
+        // end:noformat
         assertAcked(client().admin().indices().prepareCreate("foo_2").addMapping("doc", builder).get());
-        // @formatter:off
+        // tag:noformat
         XContentBuilder docBuilder = jsonBuilder().startObject()
             .startObject("license")
               .field("partnumber", "foobar")
               .field("count", 2)
             .endObject()
             .field("@timestamp", "2018-07-08T08:07:00.599Z")
           .endObject();
-        // @formatter:on
+        // end:noformat
         client().prepareIndex("foo_2", "doc").setSource(docBuilder).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).get();
 
         client().admin().indices().prepareRefresh();

diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractHyperLogLog.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractHyperLogLog.java
@@ -21,7 +21,7 @@ public abstract class AbstractHyperLogLog extends AbstractCardinalityAlgorithm {
     private static final int BIAS_K = 6;
 
     // these static tables come from the appendix of the paper
-    // @formatter:off
+    // tag::noformat
     private static final double[][] RAW_ESTIMATE_DATA = {
             // precision 4
             { 11, 11.717, 12.207, 12.7896, 13.2882, 13.8204, 14.3772, 14.9342, 15.5202, 16.161, 16.7722, 17.4636, 18.0396, 18.6766, 19.3566,
@@ -689,7 +689,7 @@ public abstract class AbstractHyperLogLog extends AbstractCardinalityAlgorithm {
                     -404.317000000039, -528.898999999976, -506.621000000043, -513.205000000075, -479.351000000024, -596.139999999898,
                     -527.016999999993, -664.681000000099, -680.306000000099, -704.050000000047, -850.486000000034, -757.43200000003,
                     -713.308999999892, } };
-    // @formatter:on
+    // end::noformat
 
     private static final long[] THRESHOLDS = new long[] {
         10,

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/protocol/xpack/graph/Vertex.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/protocol/xpack/graph/Vertex.java
@@ -168,7 +168,7 @@ public void setWeight(final double weight) {
         this.weight = weight;
     }
 
-    // @formatter:off
+    // tag::noformat
     /**
      * If the {@link GraphExploreRequest#useSignificance(boolean)} is true (the default)
      * this statistic is available.
@@ -177,12 +177,12 @@ public void setWeight(final double weight) {
      * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html"
      * >the significant_terms aggregation</a>)
      */
-    // @formatter:on
+    // end::noformat
     public long getBg() {
         return bg;
     }
 
-    // @formatter:off
+    // tag::noformat
     /**
      * If the {@link GraphExploreRequest#useSignificance(boolean)} is true (the default)
      * this statistic is available.
@@ -192,7 +192,7 @@ public long getBg() {
      * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html"
      * >the significant_terms aggregation</a>)
      */
-    // @formatter:on
+    // end::noformat
     public long getFg() {
         return fg;
     }

diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataTypes.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataTypes.java
@@ -20,7 +20,7 @@
 
 public final class DataTypes {
 
-    // @formatter:off
+    // tag::noformat
     public static final DataType UNSUPPORTED      = new DataType("UNSUPPORTED", null, 0,                 false, false, false);
 
     public static final DataType NULL             = new DataType("null",              0,                 false, false, false);
@@ -48,7 +48,7 @@ public final class DataTypes {
     // complex types
     public static final DataType OBJECT           = new DataType("object",            0,                 false, false, false);
     public static final DataType NESTED           = new DataType("nested",            0,                 false, false, false);
-    //@formatter:on
+    //end::noformat
 
     private static final Collection<DataType> TYPES = Arrays.asList(
         UNSUPPORTED,

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java
@@ -53,7 +53,7 @@
 
 public class SqlDataTypes {
 
-    // @formatter:off
+    // tag::noformat
     // date-only, time-only
     public static final DataType DATE = new DataType("DATE", null, Long.BYTES, false, false, true);
     public static final DataType TIME = new DataType("TIME", null, Long.BYTES, false, false, true);
@@ -88,7 +88,7 @@ public class SqlDataTypes {
     public static final DataType GEO_SHAPE = new DataType("geo_shape", Integer.MAX_VALUE, false, false, false);
     public static final DataType GEO_POINT = new DataType("geo_point", Double.BYTES * 2,  false, false, false);
     public static final DataType SHAPE =     new DataType("shape",     Integer.MAX_VALUE, false, false, false);
-    // @formatter:on
+    // end::noformat
 
     private static final Map<String, DataType> ODBC_TO_ES = new HashMap<>(mapSize(38));