From 358cd80d34677eb05fefcdc3eada1df78f5d16af Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 16 Dec 2021 16:18:34 +0000 Subject: [PATCH] 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. --- .editorconfig | 4 ---- CONTRIBUTING.md | 2 +- .../gradle/internal/checkstyle/SnippetLengthCheck.java | 5 ++++- .../conventions/precommit/FormattingPrecommitPlugin.java | 3 --- .../main/java/org/elasticsearch/client/graph/Vertex.java | 8 ++++---- .../src/main/java/org/elasticsearch/client/Response.java | 4 ++-- .../BucketMetricsPipeLineAggregationTestCase.java | 8 ++++---- .../search/aggregations/metrics/AbstractHyperLogLog.java | 4 ++-- .../org/elasticsearch/protocol/xpack/graph/Vertex.java | 8 ++++---- .../java/org/elasticsearch/xpack/ql/type/DataTypes.java | 4 ++-- .../org/elasticsearch/xpack/sql/type/SqlDataTypes.java | 4 ++-- 11 files changed, 25 insertions(+), 29 deletions(-) diff --git a/.editorconfig b/.editorconfig index 0c8a9dfd38ba..92111b6c074f 100644 --- 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 index 565d285646e6..55a95dfee30f 100644 --- 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/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java index 14c2b74e6e6b..7053bd162615 100644 --- a/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java +++ b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/SnippetLengthCheck.java @@ -22,9 +22,12 @@ import java.util.regex.Pattern; /** * Checks the snippets included in the docs aren't too wide to fit on * the page. + *

+ * Regions contained in the special noformat 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/build-conventions/src/main/java/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java index 17a845f166b1..453e5cb5d7e1 100644 --- a/build-conventions/src/main/java/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java +++ b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/conventions/precommit/FormattingPrecommitPlugin.java @@ -60,9 +60,6 @@ public class FormattingPrecommitPlugin implements Plugin { 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 index d7e1af6deb99..2db13014aa42 100644 --- 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 class Vertex implements ToXContentFragment { 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 class Vertex implements ToXContentFragment { * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html" * >the significant_terms aggregation) */ - // @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 class Vertex implements ToXContentFragment { * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html" * >the significant_terms aggregation) */ - // @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 index 9a1f21a6de64..9dbac4287212 100644 --- 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 class Response { * 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 class Response { + 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/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java index 243bc5682510..432b6241cf9f 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipeLineAggregationTestCase.java @@ -439,7 +439,7 @@ abstract class BucketMetricsPipeLineAggregationTestCasethe significant_terms aggregation) */ - // @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 class Vertex implements ToXContentFragment { * href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html" * >the significant_terms aggregation) */ - // @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 index 12ec07d6f297..08e25db8eba3 100644 --- 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 @@ import static java.util.stream.Collectors.toMap; 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 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 index fa4b258ad456..9bdb0ec16364 100644 --- 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 @@ import static org.elasticsearch.xpack.ql.util.CollectionUtils.mapSize; 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 ODBC_TO_ES = new HashMap<>(mapSize(38));