Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

Align with IntelliJ #146

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Conversation

sideeffffect
Copy link
Contributor

  • groupedImports = Merge to prevent conflict with IntelliJ
  • expandRelative = true to make converging with IntelliJ's Optimize imports faster

Maybe there are other changes necessary to match IntelliJ 100%, but from my experience over many months, using

OrganizeImports {
  expandRelative = true
  groupedImports = Merge
  groups = [
    "*"
    "re:(javax?|scala)\\."
  ]
}

doesn't cause any conflicts between IntelliJ and scalafix-organize-imports.

 * `groupedImports = Merge` to prevent conflict with IntelliJ
 * `expandRelative = true` to make converging with IntelliJ's _Optimize imports_ faster
@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #146 (848cb50) into master (869e427) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   90.90%   90.90%           
=======================================
  Files           4        4           
  Lines         242      242           
  Branches       12       12           
=======================================
  Hits          220      220           
  Misses         22       22           
Impacted Files Coverage Δ
...les/src/main/scala/fix/OrganizeImportsConfig.scala 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 869e427...848cb50. Read the comment docs.


import sun.misc.BASE64Encoder

import util.control
import control.NonFatal

Copy link
Owner

@liancheng liancheng Dec 28, 2020

Choose a reason for hiding this comment

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

This change violates the original motivation of the test. It meant to test whether relative imports are grouped in a separate order-preserving group, but now they are expanded.

To avoid such issues, let's not change any output files but update the OrganizeImports configurations in all the input files to reflect the new default configuration values.

@liancheng
Copy link
Owner

liancheng commented Dec 28, 2020

Thanks, @sideeffffect. I left a comment about the changes in the tests, otherwise LGTM.

I'm afraid we won't be able to be 100% compatible with IntelliJ default configurations. OrganizeImports values correctness more than conciseness because it is supposed to be used in a batch manner in CI, and we don't want to break your code there. The default IntelliJ configuration values conciseness more even though sometimes it may produce wrong results. For example:

Class count to use import with '_': 5

The above configuration is equivalent to OrganizeImports.coalesceToWildcardImportThreshold = 5, which is not recommended and may result in compilation errors in corner cases (see here). This might be acceptable for IntelliJ since its import optimizer is mostly used in interactive ways and users can spot and fix any errors on the fly.

With that said, changes proposed in this PR are safe, though.

@liancheng
Copy link
Owner

liancheng commented Dec 28, 2020

Oh, actually, another issue: defaulting expandRelative to true may produce unused imports that cannot be removed even if you set removeUnused to true. See here. This means that if you set both expandRelative and removeUnused to true, then the output may not be idempotent unless you run OrganizeImports at least twice. It was the biggest reason why I didn't enable it by default.

I'm not quite sure whether it's a good idea to enable it by default.

Maybe leaving it disabled by default is fine? Because in that case, OrganizeImports doesn't do or complain anything, so it does not conflict with IntelliJ anyway.

@liancheng
Copy link
Owner

liancheng commented Dec 28, 2020

I'm thinking maybe we can add a basedOnStyle option to provide different sets of default configurations. Say:

OrganizeImports.basedOnStyle = INTELLIJ_2020_3

implies:

OrganizeImports {
  coalesceToWildcardImportThreshold = 5
  groups = [
    "*",
    "re:(javax?|scala)\\."
  ]
  groupedImports = Merge
}

Users can still override individual options.

@sideeffffect
Copy link
Contributor Author

I've changed default expandRelative = false. I think this is good to go into master and then release. Then we can wait for feedback how it works in practice 😉

While in the future it may turn out handy to have a basedOnStyle setting, I don't think we're there yet. So far I would bet that we can get away without using it, so let's not add it just yet. We'll see how far we can get with just IntelliJ-aligned defaults after this PR is merged and a new scalafix-organize-imports version released.

@liancheng liancheng merged commit 163a3bf into liancheng:master Jan 12, 2021
@liancheng
Copy link
Owner

Thanks and merged!

liancheng added a commit that referenced this pull request Jan 12, 2021
@sideeffffect sideeffffect deleted the align-with-intellij branch January 12, 2021 10:17
@liancheng liancheng added this to the v0.5.0 milestone Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants