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

Add preset styles, including one for IntelliJ IDEA 2020.3 #154

Merged
merged 19 commits into from
Feb 1, 2021

Conversation

liancheng
Copy link
Owner

@liancheng liancheng commented Jan 16, 2021

This PR tries to address the backward-incompatible changes introduced in PR #143 and #146 by adding preset import organization styles. Now, users may configure OrganizeImports by choosing and overriding one of two preset styles using the newly introduced preset option. Two preset styles are available:

  • DEFAULT

    The default style:

    OrganizeImports {
      blankLines = Auto
      coalesceToWildcardImportThreshold = null
      expandRelative = false
      groupExplicitlyImportedImplicitsSeparately = false
      groupedImports = Explode
      groups = [
        "re:javax?\\."
        "scala."
        "*"
      ]
      importSelectorsOrder = Ascii
      importsOrder = Ascii
      removeUnused = true
    }
    
  • INTELLIJ_2020_3

    A style that is more consistent with the default configuration of the IntelliJ 2020.3 Scala import optimizer:

    OrganizeImports {
      blankLines = Manual
      coalesceToWildcardImportThreshold = 5
      expandRelative = false
      groupExplicitlyImportedImplicitsSeparately = false
      groupedImports = Merge
      groups = [
        "*"
        "---"
        "java."
        "javax."
        "scala."
      ]
      importSelectorsOrder = Ascii
      importsOrder = Ascii
      removeUnused = true
    }
    

Users can override individual configurations to fine-tune the style, e.g.:

OrganizeImports {
  preset = INTELLIJ_2020_3
  expandRelative = true
}

@liancheng liancheng added this to the v0.5.0 milestone Jan 16, 2021
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #154 (a1af21f) into branch-0.5 (574bc20) will increase coverage by 0.43%.
The diff coverage is 96.77%.

Impacted file tree graph

@@              Coverage Diff               @@
##           branch-0.5     #154      +/-   ##
==============================================
+ Coverage       91.20%   91.63%   +0.43%     
==============================================
  Files               4        4              
  Lines             250      263      +13     
  Branches            9        7       -2     
==============================================
+ Hits              228      241      +13     
  Misses             22       22              
Impacted Files Coverage Δ
rules/src/main/scala/fix/OrganizeImports.scala 96.44% <93.75%> (+0.09%) ⬆️
...les/src/main/scala/fix/OrganizeImportsConfig.scala 100.00% <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 574bc20...41e67d4. Read the comment docs.

@liancheng
Copy link
Owner Author

liancheng commented Jan 17, 2021

@sideeffffect, based on our previous discussion, I made this PR to ensure both IntelliJ compatibility and backward compatibility. The INTELLIJ_2020_3 preset style mimics IntelliJ as much as possible, even though there's a small chance to introduce correctness errors. Please let me know what you think.

I plan to have this in v0.5.0 if it looks good to you.

@liancheng liancheng changed the base branch from master to branch-0.5 January 17, 2021 06:42
Copy link
Contributor

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

Nice job @liancheng! 👍

What do you think about making INTELLIJ_2020_3 the default style? I presume that you didn't make it the default because you don't want to break current scalafix-organize-imports users (I'm one of them :) ). That is a respectable decision.

But my humble suggestion would be to be bold and change the default to INTELLIJ_2020_3 even though it's a breaking change. This plugin is still 0.x, so you have license to do that. The migration story for current users would be fair, they would either reformat their code-base (a simple, automated task) or set their configuration to preset = OLD_STYLE (or something like that). The benefit would be great: less configuration needed for scalafix-organize-imports. Now I'm not only thinking about the current users, but also about the soon to be converts that are not users yet. My belief is that most people don't care what the import organization is, as long as it is in harmony with IntelliJ, which is the most used Scala IDE after all.

groupedImports = GroupedImports.Merge,
groups = Seq(
"*",
"---",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what --- means, but I assume you know what you're doing 😄

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is introduced by a feature recently added in PR #142. Please check the doc here. In short, it allows you to customize the position of blank lines, just like what IntelliJ allows you to do. The --- is the blank line marker.

@liancheng
Copy link
Owner Author

liancheng commented Jan 18, 2021

@sideeffffect, thanks for the feedback! I thought about making INTELLIJ_2020_3 the default style but decided not to do that because of a few reasons:

  1. The default IntelliJ Scala import optimizer configuration may introduce subtle correctness issues.

    For example, coalescing grouped imports into a single wildcard import can be problematic. An example can be found here. OrganizeImports values correctness a lot. Making INTELLIJ_2020_3 the default style means either of the following two cases:

    • By default, OrganizeImports might break your CI/CD pipelines in subtle ways, which is not acceptable.
    • INTELLIJ_2020_3 cannot include any configuration that may introduce correctness issue, but that makes it less compatible with IntelliJ, which is not ideal either.
  2. The evolution of the default configuration of the IntelliJ Scala plugin is out of the control of this project.

    As you mentioned in PR Use new IntelliJ import order #143, IntelliJ Scala 2020.3 changed the default import order. That's also why the preset style name is INTELLIJ_2020_3 instead of simply INTELLIJ. Similar changes may happen again in the future. By then, we'll have to add another new preset style, but should we make that new style the default one? What if OrganizeImports is already beyond 1.x at that time?

  3. The current DEFAULT style is opinionated and more recommended for new projects.

    It is opinionated in two ways:

    • It ensures correctness whenever possible. E.g., it does not coalesce grouped imports into wildcard imports.

    • By setting groupedImports to Explode, despite making the import section lengthier, it is more friendly to version control, and less likely to introduce trivial but annoying merge conflicts involving import.

      I personally hit this kind of conflict a lot in real life and it's a huge waste of time. @olafurpg also adopted the same configuration in scalameta/metals due to the same reason.

@sideeffffect
Copy link
Contributor

@liancheng I see your points 👍 Having INTELLIJ_2020_3 available would definitely be an improvement over the status quo, thank you for it 🙇
What do you think about adopting the INTELLIJ_2020_3 import order to DEFAULT? Or would it also hamper correctness?

@niktrop @pavelfatin Would it be thinkable that IntelliJ would change (again) the defaults, to be closer to that of scalafix-organize-imports?

@pavelfatin
Copy link

Hi! Thanks for inviting us. We'd be happy to work together.

Having a reasonable common default style is important not just for each particular tool, but for all Scala programmers.

We agree that wildcard imports are problematic. We will update the style to coalesce neither import statements nor grouped imports into a wildcard. This will apply to newly added imports and won't affect already existing ones, at least but default.

On your part, it would be great if you update the sorting of groups to match the IDEA's one :) There are probably more projects formatted in this style. Besides, there are arguments why that particular sorting order may make sense in general (https://blog.jetbrains.com/scala/2020/11/26/enhanced-package-prefixes/).

All in all, deciding on the default style is not an easy task, many things should be taken into account, including: writing code, reading code, the possibility of automatic optimizations, interaction with VCS, existing projects, widely accepted variants, interaction with other language constructs, well-knows recommendations, possible downstream (butterfly) effects, etc.

BTW, how about we invite ScalaCenter, so that we can collectively work out a "standard" import style, as a part of https://docs.scala-lang.org/style ?

(Feel free to join https://gitter.im/JetBrains/intellij-scala so that we can discuss the details in a more interactive way.)

@liancheng
Copy link
Owner Author

Hey @sideeffffect and @pavelfatin, thanks for the feedback and sorry for the late reply, have been quite busy with daytime work recently.

+1 for adjusting the default value of the groups option to match IDEA. I'll merge this PR after making that adjustment.

Inviting ScalaCenter is also a great idea! Let me find some time to start a GitHub discussion thread here. I don't have a specific list of names to invite, though. Maybe @olafurpg and @mlachkar are willing to join and/or suggest candidates?

@pavelfatin
Copy link

We've asked about the best way to approach the task in Slack | intellij | ScalaCenter channel.

@liancheng
Copy link
Owner Author

@pavelfatin, cool, is it a public channel? Had a try but couldn't find it.

@liancheng liancheng merged commit a906139 into branch-0.5 Feb 1, 2021
@pavelfatin
Copy link

@liancheng That's a private channel, sorry. But I've essentially just copy/pasted the above text. The point is that we've reached out to ScalaCenter :)

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.

None yet

3 participants