Formatting escape hatch (elastic#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.
ue5377 · Dec 16, 2021 · 5b49982 · 5b49982
1 parent 82592c4
commit 5b49982
Show file tree

Hide file tree

Showing 16 changed files with 59 additions and 63 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
@@ -299,7 +299,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
@@ -440,7 +440,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")
@@ -464,17 +464,17 @@ public void testFieldIsntWrittenOutTwice() throws Exception {
               .endObject()
             .endObject()
           .endObject();
-        // @formatter:on
+        // end::noformat
         assertAcked(client().admin().indices().prepareCreate("foo_2").setMapping(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").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/...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java b/...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/TransportEqlSearchAction.java
@@ -306,9 +306,9 @@ private static void qualifyEvents(List<EqlSearchResponse.Event> events, String c
 
     private static Exception qualifyException(Exception e, String[] indices, String clusterAlias) {
         Exception finalException = e;
-        // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+        // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
         if (e instanceof RemoteTransportException && e.getCause() instanceof IndexNotFoundException infe) {
-            // @formatter:on
+            // end::noformat
             if (infe.getIndex() != null) {
                 String qualifiedIndex;
                 String exceptionIndexName = infe.getIndex().getName();

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/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
@@ -846,9 +846,9 @@ private static class ResolveFilterRefs extends AnalyzerRule<LogicalPlan> {
         @Override
         protected LogicalPlan rule(LogicalPlan plan) {
             if (plan instanceof Project p) {
-                // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+                // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
                 if (p.child() instanceof Filter f) {
-                    // @formatter:on
+                    // end::noformat
                     Expression condition = f.condition();
                     if (condition.resolved() == false && f.childrenResolved()) {
                         Expression newCondition = replaceAliases(condition, p.projections());
@@ -860,9 +860,9 @@ protected LogicalPlan rule(LogicalPlan plan) {
             }
 
             if (plan instanceof Aggregate a) {
-                // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+                // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
                 if (a.child() instanceof Filter f) {
-                    // @formatter:on
+                    // end::noformat
                     Expression condition = f.condition();
                     if (condition.resolved() == false && f.childrenResolved()) {
                         Expression newCondition = replaceAliases(condition, a.aggregates());
@@ -1029,9 +1029,9 @@ private static class HavingOverProject extends AnalyzerRule<Filter> {
 
         @Override
         protected LogicalPlan rule(Filter f) {
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if (f.child() instanceof Project p) {
-                // @formatter:on
+                // end::noformat
                 for (Expression n : p.projections()) {
                     if (n instanceof Alias) {
                         n = ((Alias) n).child();
@@ -1072,9 +1072,9 @@ protected boolean skipResolved() {
         @Override
         protected LogicalPlan rule(Filter f) {
             // HAVING = Filter followed by an Agg
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if (f.child() instanceof Aggregate agg && agg.resolved()) {
-                // @formatter:on
+                // end::noformat
                 Set<NamedExpression> missing = null;
                 Expression condition = f.condition();
 
@@ -1248,9 +1248,9 @@ public static class ReplaceSubQueryAliases extends AnalyzerRule<UnaryPlan> {
 
         @Override
         protected LogicalPlan rule(UnaryPlan plan) {
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if (plan.child() instanceof SubQueryAlias a) {
-                // @formatter:on
+                // end::noformat
                 return plan.transformExpressionsDown(FieldAttribute.class, f -> {
                     if (f.qualifier() != null && f.qualifier().equals(a.alias())) {
                         // Find the underlying concrete relation (EsIndex) and its name as the new qualifier

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
@@ -396,9 +396,9 @@ private static boolean checkGroupByHaving(
         AttributeMap<Expression> attributeRefs
     ) {
         if (p instanceof Having h) {
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if (h.child() instanceof Aggregate a) {
-                // @formatter:on
+                // end::noformat
                 Set<Expression> missing = new LinkedHashSet<>();
                 Set<Expression> unsupported = new LinkedHashSet<>();
                 Expression condition = h.condition();

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java
@@ -324,9 +324,9 @@ private void findNested(Expression exp, AttributeMap<Function> functions, Consum
         @Override
         protected LogicalPlan rule(Project project) {
             // check whether OrderBy relies on nested fields which are not used higher up
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if (project.child() instanceof OrderBy ob) {
-                // @formatter:on
+                // end::noformat
                 // resolve function references (that maybe hiding the target)
                 AttributeMap.Builder<Function> collectRefs = AttributeMap.builder();
 
@@ -1036,23 +1036,23 @@ public LogicalPlan apply(LogicalPlan p) {
 
             // count the extended stats
             p.forEachExpressionUp(InnerAggregate.class, ia -> {
-                // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+                // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
                 if (ia.outer() instanceof ExtendedStats extStats) {
                     seen.putIfAbsent(extStats.field(), extStats);
                 }
-                // @formatter:on
+                // end::noformat
             });
 
             // then if there's a match, replace the stat inside the InnerAgg
             return p.transformExpressionsUp(InnerAggregate.class, ia -> {
-                // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+                // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
                 if (ia.outer() instanceof Stats stats) {
                     ExtendedStats ext = seen.get(stats.field());
                     if (ext != null && stats.field().equals(ext.field())) {
                         return new InnerAggregate(ia.inner(), ext);
                     }
                 }
-                // @formatter:on
+                // end::noformat
                 return ia;
             });
         }
@@ -1187,7 +1187,7 @@ static class PushProjectionsIntoLocalRelation extends OptimizerRule<UnaryPlan> {
 
         @Override
         protected LogicalPlan rule(UnaryPlan plan) {
-            // @formatter:off - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
+            // tag::noformat - https://bugs.eclipse.org/bugs/show_bug.cgi?id=574437
             if ((plan instanceof Project || plan instanceof Aggregate) && plan.child() instanceof LocalRelation relation) {
                 List<Object> foldedValues = null;
 
@@ -1198,7 +1198,7 @@ protected LogicalPlan rule(UnaryPlan plan) {
                         foldedValues = extractLiterals(((Project) plan).projections());
                     }
                 } else if (relation.executable() instanceof EmptyExecutable) {
-                    // @formatter:on
+                    // end::noformat
                     if (plan instanceof Aggregate agg) {
                         if (agg.groupings().isEmpty()) {
                             // Implicit groupings on empty relations must produce a singleton result set with the aggregation results