Skip to content

Order imports when reformatting #74059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Jun 14, 2021

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.

@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jun 14, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I'm a bit on the fence on this one unless we can have this configured on import automatically. A lot of this manual IDE config stuff is missed by developers. If someone doesn't do this they are going to be in for a very frustrating time battling between IDE rules and spotless.

I'll quote David here:

yes sorting differently from IntelliJ would definitely be worse than today’s lack of sorting

We need to factor in that if this isn't configured automatically a non-negligible number of folks are going to hit this issue. I think having manual setup for things that are "optional" is fine, but I consider having import ordering rules match between the IDE and build be pretty much mandatory given that basically everyone uses this IDE feature.

@mark-vieira
Copy link
Contributor

I think we can commit some stuff to .idea/codeStyles to declare a code style config with this stuff and have intelliJ automatically pick it up on import. We can then just ditch the codeStyle { } block in elasticsearch.ide.gradle.

@mark-vieira
Copy link
Contributor

So yeah, let's set up the IDE import config the way we want, export it to a file in .idea/codeStyles and then edit .idea/codestyles/codeStyleConfig.xml to point to that config. I think that should do the trick.

@pugnascotia
Copy link
Contributor Author

@mark-vieira I tried your suggestion but struggled to get it to work. I asked in the IntelliJ forums and someone suggested using a Settings Repository. Not mad keen on introducing something new - what do you think?

@mark-vieira
Copy link
Contributor

mark-vieira commented Jun 15, 2021

Yeah, that is not gonna be a good solution given that code style is a project setting and settings repositories are for making all your IDE settings portable. I mean, we commit other stuff in the .idea folder, I feel like this should work. I wonder if we need to do something like we do for the Gradle JVM settings and modify the codeStyleConfig.xml config file on import vs check one into source control. I assume that's the issue? That the IDE is fighting with what we set there? I presume that just dropping a custom codestyle file in there get's picked up by IntelliJ just fine.

@pugnascotia
Copy link
Contributor Author

So it turns out you can configure the imports order in IntelliJ via .editorconfig, so that's what I've done for Java and Groovy.

To see all supported properties, right-click a folder and select "New | EditorConfig File", check the IntelliJ IDEA-specific checkbox in the dialog that opens and, if needed, select the languages used in your project. An .editorconfig file will be generated with all IntelliJ-specific properties listed as comments.

@pugnascotia pugnascotia requested a review from mark-vieira June 15, 2021 20:47
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant 👍

@pugnascotia pugnascotia merged commit a5d2251 into elastic:master Jun 16, 2021
@pugnascotia pugnascotia deleted the sort-imports branch June 16, 2021 08:22
pugnascotia added a commit that referenced this pull request Jun 16, 2021
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.
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in fb8f84f

limingnihao pushed a commit to limingnihao/elasticsearch that referenced this pull request Jun 17, 2021
* master: (284 commits)
  [DOCS] Update central reporting image (elastic#74195)
  [DOCS] SQL: Document `null` handing for string functions (elastic#74201)
  Fix Snapshot Docs Listing Query Params in Body Incorrectly (elastic#74196)
  [DOCS] EQL: Note EQL uses `fields` parameter (elastic#74194)
  Mute failing MixedClusterClientYamlTestSuiteIT test {p0=indices.split/20_source_mapping/Split index ignores target template mapping} test (elastic#74198)
  Cleanup Duplicate Constants in Snapshot XContent Params (elastic#74114)
  [DOC] Add watcher to the threadpool doc (elastic#73935)
  [Rest Api Compatibility] Validate Query typed api (elastic#74171)
  Replace deprecated `script.cache.*` settings with `script.context.$constext.cache_*` in documentation. (elastic#74144)
  Pin Alpine Linux version in Docker builds (elastic#74169)
  Fix clone API settings docs bug (elastic#74175)
  [ML] refactor internal datafeed management (elastic#74018)
  Disable query cache for FunctionScoreQuery and ScriptScoreQuery (elastic#74060)
  Fork the sending of file chunks during recovery (elastic#74164)
  RuntimeField.Builder should not extend FieldMapper.Builder (elastic#73840)
  Run CheckIndex on metadata index before loading (elastic#73239)
  Deprecate setting version on analyzers (elastic#74073)
  Add test with null transform id in stats request (elastic#74130)
  Order imports when reformatting (elastic#74059)
  Move deprecation code from xpack core to deprecation module. (elastic#74120)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Tooling Developer tooliing and automation >enhancement Team:Delivery Meta label for Delivery team v7.13.3 v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants