Skip to content
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

Enable "organize imports on save" #2579

Open
jukzi opened this issue Jun 17, 2024 · 8 comments
Open

Enable "organize imports on save" #2579

jukzi opened this issue Jun 17, 2024 · 8 comments

Comments

@jukzi
Copy link
Contributor

jukzi commented Jun 17, 2024

I would like to enable organize imports on save and would like to get feedback from jdt.core commiters.

Known benefit: i don't have to manually remove no longer used imports.
Known downslde: The first commit on each file will have a longer - unrelated - patch size.

optional workaround: reorganize all imports once for all files.
history: at some point of time the sort order for imports was changed.


If you start a discussion and mention me, I will add more details there. Hint: if the change of strategy is done well, I'm not strictly against it.

Originally posted by @stephan-herrmann in #1700 (comment)

@iloveeclipse
Copy link
Member

Yes, it makes sense to organize imports on save.
Not sure how many changes would happen on master, but if that action would be done for all sources would be good too.

@jjohnstn
Copy link
Contributor

I also thinks this makes sense. I would also be in favor of reorganizing the imports in a mass change.

@stephan-herrmann
Copy link
Contributor

There is one small issue, why I'm not 100% happy with automatic organize import: it doesn't respect existing wildcard imports. I know that many people are actively opposed to wildcard imports, so here is my reason for liking them: I actually look at imports once in a while to get an idea of what a class depends on. When organize imports expands a nice import org.eclipse.jdt.internal.compiler.ast.*; into 147 lines of imports, the import list is simply overwhelming / useless for the human eye.

As a workaround I hope we can agree on a threshold for automatic wildcard imports. I'd propose a value of 10.

@stephan-herrmann
Copy link
Contributor

Yes, it makes sense to organize imports on save. Not sure how many changes would happen on master, but if that action would be done for all sources would be good too.

I agree. Doing this incrementally would mean a lot of trouble with experimental changes, which trigger organize imports, to the effect that reverting the experimental change will not put the file back into a clean state.

A mass change on imports is acceptable, because import lists are not the typical subject of research into the git history :)

The typical caveat remains: we need to be 100% sure that the mass change does not introduce any unrelated changes, and it must happen in the same PR that enables automatic organize imports on save, to avoid any drift.

And a second caveat: it would create one more reason why changes must be prepared in the original Eclipse project context, to avoid any drift later on. For comparison: remove-trailing whitespace was globally enabled some years ago, and still once in a blue moon I see unrelated changes on save, because some commit succeeded to sneak in unwanted trailing white space :)

@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Jun 18, 2024

Not sure how many changes would happen on master

this raises the question: how to coordinate this with BETA_JAVA23?

  • define a baseline such that the change can be immediately propagated to the branch, or
  • exclude that commit from merging (master->BETA_JAVA23) and repeat the same procedure as was done in master. (would require some merge -s ours magic or so), or
  • wait for the moment when there is no active secondary branch <== Least effort

@jukzi
Copy link
Contributor Author

jukzi commented Jun 20, 2024

  • wait for the moment when there is no active secondary branch <== Least effort

when would that be next time and how long will be the duration?

@stephan-herrmann
Copy link
Contributor

  • wait for the moment when there is no active secondary branch <== Least effort

when would that be next time and how long will be the duration?

when: soon after Java 23 GA (2024/09)

duration: this can probably be negotiated

@stephan-herrmann
Copy link
Contributor

Organize imports, which was accidentally enabled in o.e.j.core.tests.model, just created a flood of new warnings, all of the style:

[Check warning on line 98 in org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/dom/ASTConverterAST3Test.java](https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2543/files#annotation_23093459622)

@jenkins-eclipse-jdt jenkins-eclipse-jdt / Compiler and API Tools

Deprecation

NORMAL:
The type DOMFactory is deprecated

I'll manually revert the "disorganize" :) to get a green build.

jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Aug 29, 2024
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Aug 29, 2024
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
With project specific .settings\org.eclipse.jdt.ui.prefs:

org.eclipse.jdt.ui.importorder=
sp_cleanup.organize_imports=true
org.eclipse.jdt.ui.ondemandthreshold=10
org.eclipse.jdt.ui.staticondemandthreshold=10

eclipse-jdt#2579
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
With project specific .settings\org.eclipse.jdt.ui.prefs:

org.eclipse.jdt.ui.importorder=
sp_cleanup.organize_imports=true
org.eclipse.jdt.ui.ondemandthreshold=10
org.eclipse.jdt.ui.staticondemandthreshold=10

eclipse-jdt#2579
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this issue Sep 30, 2024
jukzi pushed a commit that referenced this issue Oct 1, 2024
With project specific .settings\org.eclipse.jdt.ui.prefs:

org.eclipse.jdt.ui.importorder=
sp_cleanup.organize_imports=true
org.eclipse.jdt.ui.ondemandthreshold=10
org.eclipse.jdt.ui.staticondemandthreshold=10

#2579
jukzi pushed a commit that referenced this issue Oct 1, 2024
jukzi pushed a commit that referenced this issue Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants