-
Notifications
You must be signed in to change notification settings - Fork 455
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
Spotless with Eclipse Import Order has different ordering than Eclipse Formatter #522
Comments
Sorry I forgot to look up relevant issues, would you like me to dupe this issue out? |
I think your issue adds helpful new information. Looks like krasa/EclipseCodeFormatter#105 fixed part of this problem (so they are closer to a fix than we are), and krasa/EclipseCodeFormatter#200 fixes the rest of it. If someone ever gets around to a PR for one of these import sorter issues, I'd like to resolve all of them, to minimize back-compat issues. What I mean by back-compat is that it's important for Spotless users to have the option to freeze their formatting. Fixing this bug will change the order of imports in existing code. So it's important to have something like |
Hello. I have another case of inconsistent import order handling which is not related neither upper case packages(EclipseCodeFormatter#105) nor static imports(EclipseCodeFormatter#200). Do I need to file a new issue against it or it's ok to provide details here? |
Is the problem that it's different than eclipse? If so this issue is a good place for it. Otherwise a new issue is better. |
@nedtwigg yes it is.
Test.importorder:
The file pre-formatted with ECF with Test.importorder:
Output of
|
@nedtwigg any update on this issue? |
I have no personal plans to work on any of the importsorter issues. I don't care what the order is, I just care that something enforces it. I agree it's a bug, and I'm happy to merge a solution from anyone. |
@ekropotin we can work around this issue by adding an additional item to the
This makes spotless formatting consistent with IDEA/Eclipse code formatter, so I'm able to use save actions with reformat on save without causing any conflicts. |
@cimi This works like a charm! Thanks. You made my day! |
Good advice, however unfortunately this still doesn't seem fix the issue entirely in some cases: Eclipse:
Spotless:
Eclipse seems to order segments consisting entirely of uppercase letters below segments with mixed case, regardless of the characters. |
See diffplug/spotless#522 and diffplug/spotless#522 (comment) This fits both Eclipse and IntelliJ better. This was done with: - Eclipse organize imports with the import order for spotless - IntelliJ organize imports, this only changes 4 files - Running spotlessApply, which changes two files "back to Eclipse"
This is a bit of a confusing case, as
This is what Eclipse produces and I'd consider it correct. But this result only makes sense if you know that Regarding the fact that "Organize Imports" in Eclipse never only just sorts imports, but e.g. also turning some specific static imports into regular imports: Again, this can only be done correctly when all referenced types are available (which they're not) to determine whether Anyway, to fix the incorrect ordering of packages starting with upper case, it might be necessary to explicitly list them. Then they could be handled accordingly. |
I'm inclined to close this as "wontfix" now that we understand the problem better. As you put it @Frettman
I like your idea
Maybe a regex where if the regex matches then it is sorted as a package rather than a class? |
Despite me posting here, I don't actually have or use upper case package names and don't expect to. But I imagine (or at least hope) it's rare enough that a simple list of package names should suffice. If patterns would be really helpful for some reason, I'd evaluate if something simpler than regex would be enough: e.g.
|
I vote for the simplest thing possible. After reading your comment, I am convinced that a list of packages is the simplest thing, and regex makes it more complicated than necessary. |
Possibly another difference between Eclipse/jdt.ls and Spotless/IntelliJ, which is also related to nested classes: import static com.example.TestFixtures.A_CONST;
import static com.example.TestFixtures.NestedFoo.B_CONST;
import static com.example.TestFixtures.Z_CONST; While Spotless/IntelliJ (with import static com.example.TestFixtures.A_CONST;
import static com.example.TestFixtures.Z_CONST;
import static com.example.TestFixtures.NestedFoo.B_CONST; I think the Eclipse one looks better, though I'm atm more keen on finding a way just to make them consistent. |
The trouble is that Spotless can't tell if |
I think we should assume that people stick to the conventions and start their class names with an uppercase characters. |
But Spotless could work on a best effort basis and work under the assumption (and common convention) that package names are lower case and class names start with an upper case letter. As such, for |
While I'd personally consider changing the behavior according to @Frederick888's example a fix, do we need a flag to restore the old behavior anyway? |
I just provided a pull request. It introduces semantics-aware sorting of Java imports, i.e. imports are sorted by package, classes and static members, instead of just lexicographically. This is the new default behavior, but the old one can be restored (in Maven e.g. via false). The imports are split based on conventions: package names start with lower case, class names with upper case. For exceptions, the and configurations can be used. |
Thanks for the PR, but I just want to reiterate in this issue the same point that I made in the PR: The most important compatibility guarantee we make is to our existing users. I agree that for new users, semanticSort is probably the correct default behavior. But the premise of Spotless is "formatting doesn't matter, turn this on and then stop thinking about it". Anytime we change the defaults our users have to think about it, which is bad. |
I just tried this change (thanks!) and found a few differences compared to the eclipse formatter:
Can you tell which of these differences are expected behavior? |
@jhonnen, you definitely found a bug that explains both differences. Within each group (package, class, member) sorting should only be using the natural/lexicographical order for strings, which means upper case before lower case. Just to be clear, it was my intention that Spotless produces the same order as Eclipse here (which seems straight forward enough to be used as a general mode and not just e.g. an Eclipse-compatibility mode). @nedtwigg Should I create a new PR for the fix? |
Yes, I'd be happy to merge a PR to fix these issues. In this unique case don't add a changelog entry since we haven't released the original yet. |
Fixed in |
* update versions * remove ccv1 plugin * fix spotless importorder diffplug/spotless#522 (comment) * typo * adapt to changed @input handling of Gradle 8 https://docs.gradle.org/8.0.1/userguide/validation_problems.html#incorrect_use_of_input_annotation * use variable * bump versions * kotlin script transition WIP * kotlin script and updates * use jvm-test-suite plugin * update java version * extract commonTest to test-utils subproject * update cleanGlob * minor updates * fix deprecations and other warings * cleanup code * cleanup code * refactor mpern.sap.commerce.build.util.Extension #44 * update CHANGELOG * split into subprojects * fix reuse compliance * fix GH actions * try toolchain * disalbe SAP vendor in toolchain * fix GH actions part 2 * fix paths to test results * fix backwards compat * misc improvements * update build * whups * misc code cleanup
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.38.0` -> `2.39.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.3.0` -> `3.3.1` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.3.0` -> `3.3.1` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.39.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2390---2023-05-24) ##### Added - `Jvm.Support` now accepts `-SNAPSHOT` versions, treated as the non`-SNAPSHOT`. ([#​1583](diffplug/spotless#1583)) - Support Rome as a formatter for JavaScript and TypeScript code. Adds a new `rome` step to `javascript` and `typescript` formatter configurations. ([#​1663](diffplug/spotless#1663)) - Add semantics-aware Java import ordering (i.e. sort by package, then class, then member). ([#​522](diffplug/spotless#522)) ##### Fixed - Fixed a regression which changed the import sorting order in `googleJavaFormat` introduced in `2.38.0`. ([#​1680](diffplug/spotless#1680)) - Equo-based formatters now work on platforms unsupported by Eclipse such as PowerPC (fixes [durian-swt#​20](diffplug/durian-swt#20)) - When P2 download fails, indicate the responsible formatter. ([#​1698](diffplug/spotless#1698)) ##### Changes - Equo-based formatters now download metadata to `~/.m2/repository/dev/equo/p2-data` rather than `~/.equo`, and for CI machines without a home directory the p2 data goes to `$GRADLE_USER_HOME/caches/p2-data`. ([#​1714](diffplug/spotless#1714)) - Bump default `googleJavaFormat` version to latest `1.16.0` -> `1.17.0`. ([#​1710](diffplug/spotless#1710)) - Bump default `ktfmt` version to latest `0.43` -> `0.44`. ([#​1691](diffplug/spotless#1691)) - Bump default `ktlint` version to latest `0.48.2` -> `0.49.1`. ([#​1696](diffplug/spotless#1696)) - Dropped support for `ktlint 0.46.x` following our policy of supporting two breaking changes at a time. - Bump default `sortpom` version to latest `3.0.0` -> `3.2.1`. ([#​1675](diffplug/spotless#1675)) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v3.3.1`](quarkusio/quarkus@3.3.0...3.3.1) [Compare Source](quarkusio/quarkus@3.3.0...3.3.1) </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v3.3.1`](quarkusio/quarkus-platform@3.3.0...3.3.1) [Compare Source](quarkusio/quarkus-platform@3.3.0...3.3.1) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
If you are submitting a bug, please include the following:
Spotless with eclipse formatter orders static imports differently than eclipse or eclipse plugin in Intellij
mvn 3.3.3
1.27.0
Description: Ubuntu 16.04.6 LTS
with order
gradlew spotless[Apply/Check] --stacktrace
N/A
If you're just submitting a feature request or question, no need for the above.
EclipseFormatter issues
krasa/EclipseCodeFormatter#200
krasa/EclipseCodeFormatter#105
Write
Eclipse Plugin will order imports:
Eclipse will order imports:
and spotless will order imports
Spotless using Eclipse format rules should have the same behavior as Eclipse
The text was updated successfully, but these errors were encountered: