-
-
Notifications
You must be signed in to change notification settings - Fork 60
Add Scalafix integration #274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,3 +17,6 @@ tags | |
| .bloop/ | ||
| metals.sbt | ||
| .vscode | ||
|
|
||
| # Mac OS | ||
| .DS_Store | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| rules = [ | ||
| OrganizeImports | ||
| ] | ||
|
|
||
| OrganizeImports { | ||
| preset = INTELLIJ_2020_3 | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,19 +16,21 @@ | |
|
|
||
| package org.typelevel.sbt | ||
|
|
||
| import sbt._, Keys._ | ||
| import org.typelevel.sbt.gha.GenerativePlugin | ||
| import org.typelevel.sbt.gha.GitHubActionsPlugin | ||
| import de.heikoseeberger.sbtheader.HeaderPlugin | ||
| import org.typelevel.sbt.gha.{GenerativePlugin, GitHubActionsPlugin} | ||
| import sbt._ | ||
|
|
||
| import scala.collection.immutable | ||
|
|
||
| import Keys._ | ||
|
|
||
| object TypelevelPlugin extends AutoPlugin { | ||
|
|
||
| override def requires = | ||
| TypelevelKernelPlugin && | ||
| TypelevelSettingsPlugin && | ||
| TypelevelCiReleasePlugin && | ||
| TypelevelScalafixPlugin && | ||
| GitHubActionsPlugin && | ||
| HeaderPlugin | ||
|
|
||
|
|
@@ -73,8 +75,8 @@ object TypelevelPlugin extends AutoPlugin { | |
| }, | ||
| githubWorkflowBuild := { | ||
| WorkflowStep.Sbt( | ||
| List("headerCheckAll", "scalafmtCheckAll", "project /", "scalafmtSbtCheck"), | ||
| name = Some("Check headers and formatting"), | ||
| List("headerCheckAll", "scalafmtCheckAll", "scalafixAll --check", "project /", "scalafmtSbtCheck"), | ||
| name = Some("Check headers, migrations and formatting"), | ||
|
Comment on lines
+78
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So actually after the discussion in #247 (comment) I had an idea to do these differently. Basically, I think the This will also make it easier for an individual build to decide whether to opt-in or opt-out of any of these particular checks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, another thing to note here, is that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah of course, the semanticdb doesn't exist until then anyway! I guess it ought to be a build step then.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to work on what I described in #274 (comment), been putting it off long enough, and then we can try and re-integrate this PR against those changes. Thanks, and sorry!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a quick look at this and this is where I got to: diff --git a/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala b/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
index 335f99a..53385fa 100644
--- a/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
+++ b/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
@@ -20,33 +20,87 @@ import sbt._
import org.typelevel.sbt.gha.GenerativePlugin
import org.typelevel.sbt.gha.GitHubActionsPlugin
import org.typelevel.sbt.gha.GenerativePlugin.autoImport._
+import org.typelevel.sbt.TypelevelScalafixPlugin.autoImport._
import com.typesafe.tools.mima.plugin.MimaPlugin
import scala.language.experimental.macros
object TypelevelCiPlugin extends AutoPlugin {
- override def requires = GitHubActionsPlugin && GenerativePlugin && MimaPlugin
+ override def requires =
+ GitHubActionsPlugin && GenerativePlugin && MimaPlugin && TypelevelScalafixPlugin
+
override def trigger = allRequirements
object autoImport {
def tlCrossRootProject: CrossRootProject = macro CrossRootProjectMacros.crossRootProjectImpl
+
+ val tlCheckHeaders =
+ settingKey[Boolean]("Check for presence of copyright headers in CI (default: true)")
+ val tlCheckFormatting = settingKey[Boolean](
+ "Check code is formatted according to the project's Scalafmt config in CI (default: true)")
+ val tlCheckMigrations = settingKey[Boolean](
+ "Check configured Scalafix migrations have been applied in CI (default: true)")
}
+ import autoImport._
+
override def buildSettings = Seq(
+ tlCheckHeaders := true,
+ tlCheckFormatting := true,
+ tlCheckMigrations := true,
githubWorkflowPublishTargetBranches := Seq(),
- githubWorkflowBuild := Seq(
- WorkflowStep.Sbt(List("test"), name = Some("Test")),
- WorkflowStep.Sbt(
- List("mimaReportBinaryIssues"),
- name = Some("Check binary compatibility"),
- cond = Some(primaryJavaCond.value)
- ),
- WorkflowStep.Sbt(
- List("doc"),
- name = Some("Generate API documentation"),
- cond = Some(primaryJavaCond.value)
+ githubWorkflowBuild := {
+ val checkHeadersCommands =
+ if (tlCheckHeaders.value) List("headerCheckAll") else Nil
+
+ val checkFormattingCommands =
+ if (tlCheckFormatting.value)
+ List("scalafmtCheckAll", "project /", "scalafmtSbtCheck")
+ else
+ Nil
+
+ val checkMigrationsStep =
+ if (tlCheckMigrations.value)
+ Seq(
+ WorkflowStep.Sbt(
+ List("scalafixAll --check"),
+ name = Some("Check Scalafix migrations"),
+ cond = Some(primaryJavaCond.value)
+ )
+ )
+ else Nil
+
+ val preBuildSteps = Seq(
+ WorkflowStep.Sbt(
+ checkHeadersCommands ++ checkFormattingCommands,
+ name = Some("Check headers and formatting"),
+ cond = Some(primaryJavaCond.value)
+ )
)
- ),
+
+ val buildStep = Seq(
+ WorkflowStep.Sbt(List("test"), name = Some("Test"))
+ )
+
+ val postBuildSteps = checkMigrationsStep ++ Seq(
+ WorkflowStep.Sbt(
+ List("mimaReportBinaryIssues"),
+ name = Some("Check binary compatibility"),
+ cond = Some(primaryJavaCond.value)
+ ),
+ WorkflowStep.Sbt(
+ List("doc"),
+ name = Some("Generate API documentation"),
+ cond = Some(primaryJavaCond.value)
+ )
+ )
+
+ Seq(
+ preBuildSteps,
+ buildStep,
+ postBuildSteps
+ ).flatten
+ },
githubWorkflowJavaVersions := Seq(JavaSpec.temurin("8"))
)
@@ -54,5 +108,4 @@ object TypelevelCiPlugin extends AutoPlugin {
val java = githubWorkflowJavaVersions.value.head
s"matrix.java == '${java.render}'"
}
-
}I need to take a break now though and I seem to have lost the artifact upload steps, so I think I will wait to see what your changes look like instead 😆 |
||
| cond = Some(primaryJavaCond.value) | ||
| ) +: githubWorkflowBuild.value | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ val modules = List( | |
| "site", | ||
| "sonatype", | ||
| "sonatype-ci-release", | ||
| "scalafix", | ||
| "versioning" | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../scalafix/build.sbt |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.10.0") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /* | ||
| * Copyright 2022 Typelevel | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.typelevel.sbt | ||
|
|
||
| import sbt._ | ||
| import scalafix.sbt.ScalafixPlugin | ||
|
|
||
| import Keys._ | ||
| import ScalafixPlugin.autoImport._ | ||
|
|
||
| object TypelevelScalafixPlugin extends AutoPlugin { | ||
|
|
||
| override def requires = ScalafixPlugin | ||
|
|
||
| override def trigger = allRequirements | ||
|
|
||
| object autoImport {} | ||
|
|
||
| import autoImport._ | ||
|
|
||
| override def buildSettings = Seq[Setting[_]]( | ||
| semanticdbEnabled := true, | ||
| semanticdbVersion := scalafixSemanticdb.revision, | ||
| scalafixScalaBinaryVersion := CrossVersion.binaryScalaVersion(scalaVersion.value), | ||
| scalafixDependencies ++= Seq( | ||
| "com.github.liancheng" %% "organize-imports" % "0.6.0" | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summoning @rossabaker to tell us how to organize our imports then @valencik can put it into typelevel.g8.
Looking at http4s I guess we are just using the default configuration, since that is best for diffs and merges?
https://github.com/http4s/http4s/blob/9ac020a92409ce86ad8d67c08ba4dcd31bb6438b/.scalafix.conf
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose this configuration because although LSP users can
Organize Importsstraight from their editor, there will always be folks using IntelliJ which has its own way of handling import organisation, so this seems like a good choice to minimise friction for contributors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might otherwise agree, but the IntelliJ preset's correctness issue concerns me. And since most projects I maintain end up with maintenance branches, I appreciate the care the defaults put into diff reduction.
I won't die on this hill, though.