Merge OrganizeImports 0.6.0+37-596459af into Scalafix as a built-in rule

[liancheng]

Per the discussion in liancheng/scalafix-organize-imports#215, this PR tries to merge the OrganizeImports rule into Scalafix as a built-in rule.

TODO:

• Move rule implementation
• Update code comments referring PR/issues in https://github.com/liancheng/scalafix-organize-imports
• Apply changes made in Test Scala 3 support using sbt-projectmatrix liancheng/scalafix-organize-imports#179 to the Scalafix sbt build (better test infrastructure for Scala 3 #1528)
• Move the test infra and test cases
• Documentation
• Warn and shadow the original OrganizeImports rule under the com.github.liancheng package

Bonus:

• keep git history
• fail if RemoveUnused.imports & OrganizeImports.removeUnused are both enabled
• Run scalafix with the local OrganizeImports rule on the scalafix codebase

[matthughes]

Any updated status on this? The referenced issue #1528 appears merged. What is remaining?

[daddykotex]

I'd be happy to pick up the work on this if there is anything left to be done

[tgodzik]

Not 100% sure what needs to be done, but probably if you are able to finish the rest of TODOs and raise another PR that would be great! @liancheng I worry might not have enough time on his hands to respond

@bjaglin what do you think? What do we need here to be done still?

[liancheng]

@tgodzik, sorry for the late response, but yeah, I'm afraid I no longer have cycles to maintain this project, and I don't have enough expertise to address Scala 3 related issues. I believe merging this rule into Scalafix is the best choice for the community.

[tgodzik]

@daddykotex I think the remaining points need to be addressed so:

• Move the test infra and test cases
• Documentation

Otherwise a stretch goals would be:

• Warn and shadow the original OrganizeImports rule under the com.github.liancheng package (probably needs to be reported in the original rule)
• Bonus: fail if RemoveUnused.imports & OrganizeImports.removeUnused are both enabled

but I think two first points would be enough unless anyone else disagrees?

[bjaglin]

Sorry for not getting back to you despite offering your help @daddykotex. I finally found some time to integrate the tests, the doc (asciidoc2markdown & a bunch of manual updates), as well as the git history (all changes happen in a merge commit so git blame does work quite OK), so the best at this point would be a review.

The GitHub UI does not play very well with merge commits as the diff is only done with the main parent, but you should be able to see the actual diff compared to the original repo by looking at liancheng@b10f4f2 with a local git tool git show b10f4f289d0b93741dabb916dbf01168c264ae74 -c --combined-all-paths -M10 --patience -w seems to do the trick. Transformed with sed 's/^ -/-/' | sed 's/^ +/ /' | sed 's/^+ / /' | sed 's/^++/+/', it gives a somewhat readable diff.

Warn and shadow the original OrganizeImports rule under the com.github.liancheng package (probably needs to be reported in the original rule)

I ran a quick test to ensure that having 2 OrganizeImports available in the classpath for the ServiceLoader does not result in any error, and that the built-in rule is favored. So this could go as-is.

I would still like to add a check to avoid the compatibility warning when having com.github.liancheng::organize-imports in the classpath (which is the case for many, if not most Scalafix users), as it is now even more of a false positive since the community rule is no longer executed despite being loaded:

[info] Loading external rule(s) built against an old version of Scalafix (0.9.24).
[info] This might not be a problem, but in case you run into unexpected behavior, you
[info] should try a more recent version of the rules(s) if available. If that does
[info] not help, request the rule(s) maintainer to build against Scalafix 0.10.4
[info] or later, and downgrade Scalafix to 0.9.x (x>=24) for the time being.

I'll see if I get the time to do that this week, before the upcoming 0.11.0 release just waiting for the Scala 2.x patch releases.

[bjaglin]

bjaglin changed the title Merge OrganizeImports 0.6.0 into Scalafix as a built-in rule Merge OrganizeImports 0.6.0+37-596459af into Scalafix as a built-in rule

It turns out that the imported rule contains a (cool) unreleased bugfix 💪