Document local rules for sbt builds #1173

bjaglin · 2020-06-21T22:41:32Z

Merge only when scalacenter/sbt-scalafix#121 is released

This is the public documentation for scalacenter/sbt-scalafix#100. It's a bit late as I found scalacenter/sbt-scalafix#121 to be key for any non-2.12 project to use it. Since the original PR was not thoroughly reviewed/advertized, I guess it would be nice to iterate on the implementation itself (that we have been using internally with ScalafixTestkitPlugin on a multi-project build), as documentation highlights possible improvements & shortcomings.

I decided to create a separate section as a first step, since integrating it in the current tutorial was quite demanding. Depending on the main target for Scalafix rules development (lib or application authors?), this could become the main tutorial.

bjaglin · 2020-06-21T23:02:46Z

docs/developers/local-rules.md

+  .dependsOn(`scalafix-rules` % ScalafixConfig)
+lazy val `scalafix-input` = (project in file("scalafix/input"))
+  .disablePlugins(ScalafixPlugin)
+lazy val `scalafix-output` = (project in file("scalafix/output"))
+  .disablePlugins(ScalafixPlugin)
+lazy val `scalafix-rules` = (project in file("scalafix/rules"))
+  .disablePlugins(ScalafixPlugin)
+  .settings(
+    libraryDependencies +=
+      "ch.epfl.scala" %%
+        "scalafix-core" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion
+  )
+lazy val `scalafix-tests` = (project in file("scalafix/tests"))
+  .settings(
+    libraryDependencies +=
+      "ch.epfl.scala" %
+        "scalafix-testkit" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion %
+        Test cross CrossVersion.full,
+    scalafixTestkitOutputSourceDirectories :=
+      sourceDirectories.in(`scalafix-output`, Compile).value,
+    scalafixTestkitInputSourceDirectories :=
+      sourceDirectories.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputClasspath :=
+      fullClasspath.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputScalacOptions :=
+      scalacOptions.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputScalaVersion :=
+      scalaVersion.in(`scalafix-input`, Compile).value
+  )
+  .dependsOn(`scalafix-input`, `scalafix-rules`)
+  .enablePlugins(ScalafixTestkitPlugin)


diff is nice but hard to copy/paste, so I wonder if it's not better to leave it raw when 95% of the lines are additions, and the snippet is big

I think we could have the final file here (instead of the diff).
By the way, I like your comments/questions you add on your own PR, I find this quite useful and helpful for the review.

By the way, I like your comments/questions you add on your own PR, I find this quite useful and helpful for the review.

Thanks! However, sometimes they give context that should be captured in the git history as a source/commit comment instead, so I am trying to always question them since they can be an anti-pattern for long-term maintainability.

bjaglin · 2020-06-21T23:18:35Z

docs/developers/local-rules.md

+## Single-project sbt build
+
+If you do not have any sub-project in your build, custom rules should be
+defined under the Scalafix Ivy configuration sources directory, which by


we found this to be a problem with IntelliJ's built-in compiler (for any integration, not just single-project), as unknown Ivy configurations fallback to Compile without any proper eviction, potentially resulting in different versions of the same artifact in the classpath. I'll follow up in another PR with some notes & recommendations when that's a problem.

bjaglin · 2020-06-21T23:21:43Z

docs/developers/local-rules.md

+    libraryDependencies +=
+      "ch.epfl.scala" %%
+        "scalafix-core" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion %
+        ScalafixConfig


I guess we could make this automatic, as long as we update https://github.com/scalacenter/sbt-scalafix/blob/60199fdae7bb9c56b9d09969b0889923a6b39108/src/main/scala/scalafix/sbt/ScalafixPlugin.scala#L201-L202 to avoid always invalidating the default tool classpath. Thoughts?

I don't get it. It's normal to add scalafix-core to library dependencies?

It would be OK to add scalafix-core to library dependencies in the ScalafixConfig Ivy configuration, since anything there is meant to be rules, which require types from there.

Another downside is that it would potentially expose classpath collisions in IntelliJ mentioned in https://github.com/scalacenter/scalafix/pull/1173/files#r443266186 to all plugin users, whether or not they use local rules.

bjaglin · 2020-06-21T23:23:32Z

docs/developers/local-rules.md

+│   │   ├── Test1.scala
+│   │   ├── Test2.scala
+│   │   └── ...
+│   ├── rules/src


this could be

Suggested change

+│ ├── rules/src

+│ ├── rules/scalafix

to potentially benefit from automatic addition of scalafix-core, but it would drift from the current template and would make the sbt dependsOn more awkward (scalafix->scalafix), so I decided to stick to it.

mlachkar

I think for the last build.sbt, we could have the entire file, but it's optional.
Otherwise, I find this documentation clear and exhaustive.
Thanks

.

mlachkar · 2020-06-22T08:20:56Z

docs/developers/local-rules.md

+  .dependsOn(`scalafix-rules` % ScalafixConfig)
+lazy val `scalafix-input` = (project in file("scalafix/input"))
+  .disablePlugins(ScalafixPlugin)
+lazy val `scalafix-output` = (project in file("scalafix/output"))
+  .disablePlugins(ScalafixPlugin)
+lazy val `scalafix-rules` = (project in file("scalafix/rules"))
+  .disablePlugins(ScalafixPlugin)
+  .settings(
+    libraryDependencies +=
+      "ch.epfl.scala" %%
+        "scalafix-core" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion
+  )
+lazy val `scalafix-tests` = (project in file("scalafix/tests"))
+  .settings(
+    libraryDependencies +=
+      "ch.epfl.scala" %
+        "scalafix-testkit" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion %
+        Test cross CrossVersion.full,
+    scalafixTestkitOutputSourceDirectories :=
+      sourceDirectories.in(`scalafix-output`, Compile).value,
+    scalafixTestkitInputSourceDirectories :=
+      sourceDirectories.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputClasspath :=
+      fullClasspath.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputScalacOptions :=
+      scalacOptions.in(`scalafix-input`, Compile).value,
+    scalafixTestkitInputScalaVersion :=
+      scalaVersion.in(`scalafix-input`, Compile).value
+  )
+  .dependsOn(`scalafix-input`, `scalafix-rules`)
+  .enablePlugins(ScalafixTestkitPlugin)


I think we could have the final file here (instead of the diff).
By the way, I like your comments/questions you add on your own PR, I find this quite useful and helpful for the review.

mlachkar · 2020-06-22T08:27:31Z

docs/developers/local-rules.md

+    libraryDependencies +=
+      "ch.epfl.scala" %%
+        "scalafix-core" %
+        _root_.scalafix.sbt.BuildInfo.scalafixVersion %
+        ScalafixConfig


I don't get it. It's normal to add scalafix-core to library dependencies?

bjaglin · 2020-06-24T07:42:16Z

For information, I am going to write an article on my company's blog https://medium.com/teads-engineering to explain in details our usage Scalafix and what lead to this feature upstream (and the others I contributed, but I feel like this one is a bit more special). Maybe this is something we can integrate also, either here or in an upcoming 1.0 release post?

add missing scala 2.13 support reference

09edb4e

bjaglin force-pushed the local-rules branch 2 times, most recently from 22c30a2 to 7f89d37 Compare June 21, 2020 22:59

bjaglin commented Jun 21, 2020

View reviewed changes

bjaglin force-pushed the local-rules branch from 7f89d37 to 1569452 Compare June 21, 2020 23:05

document local rules

c0c9e88

bjaglin force-pushed the local-rules branch from 1569452 to c0c9e88 Compare June 21, 2020 23:12

bjaglin marked this pull request as ready for review June 21, 2020 23:14

bjaglin requested review from olafurpg and mlachkar June 21, 2020 23:14

bjaglin commented Jun 21, 2020

View reviewed changes

mlachkar approved these changes Jun 22, 2020

View reviewed changes

bjaglin mentioned this pull request Jun 22, 2020

allow usage of rules compiled against 2.11 & 2.13 scalacenter/sbt-scalafix#121

Merged

5 tasks

bjaglin added the documentation label Jun 30, 2020

bjaglin merged commit 05c2ad1 into scalacenter:master Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document local rules for sbt builds #1173

Document local rules for sbt builds #1173

bjaglin commented Jun 21, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

mlachkar Jun 22, 2020

bjaglin Jun 22, 2020

bjaglin Jun 21, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

mlachkar Jun 22, 2020

bjaglin Jun 22, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

mlachkar left a comment

mlachkar Jun 22, 2020

mlachkar Jun 22, 2020

bjaglin commented Jun 24, 2020 •

edited

Loading

Document local rules for sbt builds #1173

Document local rules for sbt builds #1173

Conversation

bjaglin commented Jun 21, 2020 • edited Loading

bjaglin Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

mlachkar Jun 22, 2020

Choose a reason for hiding this comment

bjaglin Jun 22, 2020

Choose a reason for hiding this comment

bjaglin Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

bjaglin Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

mlachkar Jun 22, 2020

Choose a reason for hiding this comment

bjaglin Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

bjaglin Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

mlachkar left a comment

Choose a reason for hiding this comment

mlachkar Jun 22, 2020

Choose a reason for hiding this comment

mlachkar Jun 22, 2020

Choose a reason for hiding this comment

bjaglin commented Jun 24, 2020 • edited Loading

bjaglin commented Jun 21, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

bjaglin Jun 22, 2020 •

edited

Loading

bjaglin Jun 21, 2020 •

edited

Loading

bjaglin commented Jun 24, 2020 •

edited

Loading