Skip to content

Add Scalafix integration - #283

Merged
armanbilge merged 11 commits into
typelevel:series/0.4from
armanbilge:feature/scalafix
Jun 1, 2022
Merged

Add Scalafix integration#283
armanbilge merged 11 commits into
typelevel:series/0.4from
armanbilge:feature/scalafix

Conversation

@armanbilge

@armanbilge armanbilge commented May 30, 2022

Copy link
Copy Markdown
Member

Backporting #274 into series/0.4. Closes #236. Closes #274.

Comment thread build.sbt
Comment on lines +18 to +20
ThisBuild / scalafixDependencies ++= Seq(
"com.github.liancheng" %% "organize-imports" % "0.6.0"
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Among other reasons not to tie sbt-typelevel compatibility to the organize-imports version, it hasn't been released against the latest scalafix yet.

Comment on lines +30 to +32
semanticdbEnabled := true,
semanticdbVersion := scalafixSemanticdb.revision,
scalafixScalaBinaryVersion := CrossVersion.binaryScalaVersion(scalaVersion.value),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Question: why doesn't the scalafix plugin do these settings?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm really not sure - I suppose they are thinking about folks who only want to enable Scalafix selectively, but surely they would just disable the Scalafix plugin instead

override def buildSettings = Seq[Setting[_]](
semanticdbEnabled := true,
semanticdbVersion := scalafixSemanticdb.revision,
scalafixScalaBinaryVersion := scalaBinaryVersion.value,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The docs say:

    val scalafixScalaBinaryVersion: SettingKey[String] =
      settingKey[String](
        "The Scala binary version used for scalafix execution. Must be set in ThisBuild. "
          + "Defaults to 2.12. Rules must be compiled against that binary version, or for "
          + "advanced rules such as ExplicitResultTypes which have a full cross-version, "
          + "against the corresponding full version that scalafix is built against."
      )

I'm skeptical if we should set this value to "3", ever.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did a survey of projects using scalafix and not all of them set this. For example, it seems the organize imports rule works fine with the default "2.12". So let's remove it for now.

Suggested change
scalafixScalaBinaryVersion := scalaBinaryVersion.value,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, organize imports isn't even published for Scala 3.
https://repo1.maven.org/maven2/com/github/liancheng/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the binary version is important for semantic rules (and I suppose organize imports is semantic), but I guess perhaps for the purposes of organizing plain imports the differences between Scala versions are not that relevant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I think the reason for the Scalafix binary version is for rules which directly use scalac APIs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, thanks, that was helpful.

In order not to break compatibility, while scalafix is still v0.9.x, the scalafixScalaBinaryVersion is set by default to 2.12. In other words, if this key is not specified, 2.12 rules are still being used. But going further, and starting v1, our goal is to change the default value to follow the target sources version.

So it sounds like when the time is right, Scalafix itself will change this default. We could do it too, when enough rules are published against Scala 3.

@DavidGregory084

Copy link
Copy Markdown
Contributor

Thanks for backporting this @armanbilge! Sorry for holding things up - took advantage of public holidays to get a full week off 😆

@armanbilge

Copy link
Copy Markdown
Member Author

@DavidGregory084 good for you!! 😁 Not at all, I appreciate all your work here and on the snapshot API PRs, they've turned out quite nicely :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate with scalafix in core sbt-typelevel plugin?

2 participants