Skip to content

Commit

Permalink
Order imports when reformatting (#74059)
Browse files Browse the repository at this point in the history
Change the formatter config to sort / order imports, and reformat the
codebase. We already had a config file for Eclipse users, so Spotless now
uses that.

The "Eclipse Code Formatter" plugin ought to be able to use this file as
well for import ordering, but in my experiments the results were poor.
Instead, use IntelliJ's `.editorconfig` support to configure import
ordering.

I've also added a config file for the formatter plugin.

Other changes:
   * I've quietly enabled the `toggleOnOff` option for Spotless. It was
     already possible to disable formatting for sections using the markers
     for docs snippets, so enabling this option just accepts this reality
     and makes it possible via `formatter:off` and `formatter:on` without
     the restrictions around line length. It should still only be used as
     a very last resort and with good reason.
   * I've removed mention of the `paddedCell` option from the contributing
     guide, since I haven't had to use that option for a very long time. I
     moved the docs to the spotless config.
  • Loading branch information
pugnascotia committed Jun 16, 2021
1 parent ebaa0f0 commit fb8f84f
Show file tree
Hide file tree
Showing 283 changed files with 660 additions and 578 deletions.
8 changes: 8 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,20 @@ trim_trailing_whitespace = true
insert_final_newline = true
indent_style = space

# IntelliJ-specific options
ij_java_class_count_to_use_import_on_demand = 999
ij_java_names_count_to_use_import_on_demand = 999
ij_java_imports_layout = *,|,com.**,|,org.**,|,java.**,javax.**,|,$*

[*.gradle]
indent_size = 2

[*.groovy]
indent_size = 4
max_line_length = 140
ij_groovy_class_count_to_use_import_on_demand = 999
ij_groovy_names_count_to_use_import_on_demand = 999
ij_groovy_imports_layout = *,|,com.**,|,org.**,|,java.**,javax.**,|,$*

[*.java]
indent_size = 4
Expand Down
14 changes: 14 additions & 0 deletions .idea/eclipseCodeFormatter.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 14 additions & 26 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,12 @@ action is required.
#### Formatting

We are in the process of migrating towards automatic formatting Java file
using [spotless], backed by the Eclipse formatter. If you have the [Eclipse
Code Formatter] installed, you can apply formatting directly in IntelliJ.
using [spotless], backed by the Eclipse formatter. **We strongly recommend
installing using the [Eclipse Code Formatter] plugin** so that you can
apply the correct formatting directly in IntelliJ. The configuration for
the plugin is held in `.idea/eclipseCodeFormatter.xml` and should be
automatically applied, but manual instructions are below in case you you
need them.

1. Open **Preferences > Other Settings > Eclipse Code Formatter**
2. Click "Use the Eclipse Code Formatter"
Expand All @@ -194,7 +198,8 @@ Code Formatter] installed, you can apply formatting directly in IntelliJ.

Note that only some sub-projects in the Elasticsearch project are currently
fully-formatted. You can see a list of project that **are not**
automatically formatted in [gradle/formatting.gradle](gradle/formatting.gradle).
automatically formatted in
[build-tools-internal/src/main/groovy/elasticsearch.formatting.gradle](build-tools-internal/src/main/groovy/elasticsearch.formatting.gradle).

### Importing the project into Eclipse

Expand Down Expand Up @@ -270,12 +275,10 @@ form.

### Java Language Formatting Guidelines

Java files in the Elasticsearch codebase are formatted with the Eclipse JDT
formatter, using the [Spotless
Gradle](https://github.com/diffplug/spotless/tree/master/plugin-gradle)
plugin. This plugin is configured on a project-by-project basis, via
`build.gradle` in the root of the repository. The formatting check can be
run explicitly with:
Java files in the Elasticsearch codebase are automatically formatted using
the [Spotless Gradle] plugin. All new projects are automatically formatted,
while existing projects are gradually being opted-in. The formatting check
can be run explicitly with:

./gradlew spotlessJavaCheck

Expand Down Expand Up @@ -315,28 +318,12 @@ Please follow these formatting guidelines:

IntelliJ IDEs can
[import](https://blog.jetbrains.com/idea/2014/01/intellij-idea-13-importing-code-formatter-settings-from-eclipse/)
the same settings file, and / or use the [Eclipse Code
Formatter](https://plugins.jetbrains.com/plugin/6546-eclipse-code-formatter)
plugin.
the same settings file, and / or use the [Eclipse Code Formatter] plugin.

You can also tell Spotless to [format a specific
file](https://github.com/diffplug/spotless/tree/master/plugin-gradle#can-i-apply-spotless-to-specific-files)
from the command line.

#### Formatting failures

Sometimes Spotless will report a "misbehaving rule which can't make up its
mind" and will recommend enabling the `paddedCell()` setting. If you
enabled this setting and run the format check again,
Spotless will write files to
`$PROJECT/build/spotless-diagnose-java/` to aid diagnosis. It writes
different copies of the formatted files, so that you can see how they
differ and infer what is the problem.

The `paddedCell()` option is disabled for normal operation so that any
misbehaviour is detected, and not just suppressed. You can enabled the
option from the command line by running Gradle with `-Dspotless.paddedcell`.

### Javadoc

Good Javadoc can help with navigating and understanding code. Elasticsearch
Expand Down Expand Up @@ -730,3 +717,4 @@ repeating in this section because it has come up in this context.
[Checkstyle]: https://plugins.jetbrains.com/plugin/1065-checkstyle-idea
[spotless]: https://github.com/diffplug/spotless
[Eclipse Code Formatter]: https://plugins.jetbrains.com/plugin/6546-eclipse-code-formatter
[Spotless Gradle]: https://github.com/diffplug/spotless/tree/master/plugin-gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
import org.apache.lucene.store.MMapDirectory;
import org.apache.lucene.util.IOUtils;
import org.elasticsearch.Version;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.common.lucene.search.function.ScriptScoreQuery;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.List;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.index.fielddata.IndexFieldDataCache;
import org.elasticsearch.index.fielddata.IndexNumericFieldData;
import org.elasticsearch.index.mapper.MappedFieldType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
import org.elasticsearch.Version;
import org.elasticsearch.common.breaker.CircuitBreaker;
import org.elasticsearch.common.breaker.PreallocatedCircuitBreakerService;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.Releasables;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.PageCacheRecycler;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.Releasables;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.analysis.NamedAnalyzer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,23 @@ subprojects {
target 'src/**/*.java'
}

toggleOffOn('formatter:off', 'formatter:on') // use `formatter:off` and `formatter:on` to toggle formatting - ONLY IF STRICTLY NECESSARY
removeUnusedImports()
importOrderFile rootProject.file('build-tools-internal/elastic.importorder')
eclipse().configFile rootProject.file('build-tools-internal/formatterConfig.xml')
trimTrailingWhitespace()

// See CONTRIBUTING.md for details of when to enabled this.
// Sometimes Spotless will report a "misbehaving rule which can't make up its
// mind" and will recommend enabling the `paddedCell()` setting. If you
// enabled this setting and run the format check again,
// Spotless will write files to
// `$PROJECT/build/spotless-diagnose-java/` to aid diagnosis. It writes
// different copies of the formatted files, so that you can see how they
// differ and infer what is the problem.

// The `paddedCell()` option is disabled for normal operation so that any
// misbehaviour is detected, and not just suppressed. You can enabled the
// option from the command line by running Gradle with `-Dspotless.paddedcell`.
if (System.getProperty('spotless.paddedcell') != null) {
paddedCell()
}
Expand Down
5 changes: 0 additions & 5 deletions build-tools-internal/src/main/groovy/elasticsearch.ide.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ if (System.getProperty('idea.active') == 'true') {
taskTriggers {
afterSync tasks.named('configureIdeCheckstyle'), tasks.named('configureIdeaGradleJvm'), tasks.named('buildDependencyArtifacts')
}
codeStyle {
java {
classCountToUseImportOnDemand = 999
}
}
encodings {
encoding = 'UTF-8'
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.Command;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.common.hash.MessageDigests;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.common.xcontent.XContentGenerator;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.core.SuppressForbidden;

import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.env.Environment;

import java.nio.file.Files;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,19 @@

package org.elasticsearch.common.settings;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.KeyStoreAwareCommand;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.env.Environment;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;

/**
* A sub-command for the keystore cli to create a new keystore.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@

package org.elasticsearch.common.settings;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import joptsimple.OptionSet;

import org.elasticsearch.cli.Terminal;
import org.elasticsearch.env.Environment;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

/**
* A subcommand for the keystore cli to list all settings in the keystore.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
*/
package org.elasticsearch.bootstrap;

import org.elasticsearch.common.settings.KeyStoreWrapperTests;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.common.settings.KeyStoreCommandTestCase;
import org.elasticsearch.common.settings.KeyStoreWrapper;
import org.elasticsearch.common.settings.KeyStoreWrapperTests;
import org.elasticsearch.common.settings.SecureSettings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.junit.After;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;

import org.apache.lucene.util.LuceneTestCase;
import org.elasticsearch.cli.CommandTestCase;
import org.elasticsearch.core.PathUtilsForTesting;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,6 @@
import org.junit.After;
import org.junit.Before;

import javax.crypto.Cipher;
import javax.crypto.CipherOutputStream;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

import java.io.ByteArrayOutputStream;
import java.io.DataOutputStream;
import java.io.EOFException;
Expand All @@ -49,6 +41,14 @@
import java.util.List;
import java.util.Locale;

import javax.crypto.Cipher;
import javax.crypto.CipherOutputStream;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
package org.elasticsearch.tools.launchers;

import com.sun.management.OperatingSystemMXBean;

import org.elasticsearch.tools.java_version_checker.JavaVersion;
import org.elasticsearch.tools.java_version_checker.SuppressForbidden;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,19 @@

package org.elasticsearch.tools.launchers;

import org.elasticsearch.tools.launchers.BootstrapJvmOptions.PluginInfo;

import java.util.Arrays;
import java.util.List;
import java.util.Properties;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertThat;

import java.util.Arrays;
import java.util.List;
import java.util.Properties;

import org.elasticsearch.tools.launchers.BootstrapJvmOptions.PluginInfo;

public class BootstrapJvmOptionsTests extends LaunchersTestCase {

public void testGenerateOptionsHandlesNoPlugins() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
package org.elasticsearch.tools.launchers;

import org.elasticsearch.tools.java_version_checker.JavaVersion;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.apache.lucene.search.spell.LevenshteinDistance;
import org.apache.lucene.util.CollectionUtil;
import org.apache.lucene.util.Constants;
Expand All @@ -26,18 +27,18 @@
import org.bouncycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider;
import org.elasticsearch.Build;
import org.elasticsearch.Version;
import org.elasticsearch.jdk.JarHell;
import org.elasticsearch.bootstrap.PluginPolicyInfo;
import org.elasticsearch.bootstrap.PolicyUtil;
import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.common.hash.MessageDigests;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.common.hash.MessageDigests;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.env.Environment;
import org.elasticsearch.jdk.JarHell;

import java.io.BufferedReader;
import java.io.IOException;
Expand Down
Loading

0 comments on commit fb8f84f

Please sign in to comment.