Skip to content
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

Document local rules for sbt builds #1173

Merged
merged 2 commits into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions docs/developers/before-you-begin.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,12 @@ then you might want to be more careful with corner cases.

Is your rule specific to a particular codebase or is the rule intended to be
used for any arbitrary project? It's easier to validate a rule if it will only
run on a single codebase. You may not even need tests, since the codebase is the
only test. If your rule is intended to be used in any random codebase, you need
to put more effort into handling corner cases. In general, the smaller the
target domain of your rule, the easier it is to implement a rule.
run on a single codebase. You may not even need tests, since the codebase is
the only test. In that case, you may also develop your rules [as part of the
same build as your codebase](local-rules.md), to avoid versioning & publishing
them. If your rule is intended to be used in any random codebase, you need to
put more effort into handling corner cases. In general, the smaller the target
domain of your rule, the easier it is to implement a rule.

## How often will your rule run?

Expand Down
212 changes: 212 additions & 0 deletions docs/developers/local-rules.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
---
id: local-rules
title: Local rules
---

If your custom rules only target a single codebase or repository, you can
define and develop these rules as part of the same sbt build. This way, you do
not have to version your rules and publish them to a Maven or Ivy repository.

When implementing rules that are very specific to your domain, this strategy
can be interesting as it allows rules to be added or extended in the same
change sets as the domain sources they will be run against.

## Prerequisite

Make sure the sbt plugin, as well as the Scala compiler plugin and options [are
set up correctly](../users/installation.md#sbt).

Although it is possible to define your rules in a Scala binary version that
does not match your build, it is highly recommended that you align them via:

```diff
// build.sbt
+scalafixScalaBinaryVersion in ThisBuild :=
+ CrossVersion.binaryScalaVersion(scalaVersion.value)
```

> Note that any potential external Scalafix rule, loaded with the
> `scalafixDependencies` setting key, must be built and published against the
> same Scala binary version.

## 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
Copy link
Collaborator Author

@bjaglin bjaglin Jun 21, 2020

Choose a reason for hiding this comment

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

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.

default is `src/scalafix/scala`.

```diff
repository
├── build.sbt
├── project
│   └── plugins.sbt
└── src
├── main
│   └── ...
├── test
│   └── ...
+ └── scalafix
+ ├── resources/META-INF/services
+ │   └── scalafix.v1.Rule // not mandatory, but allows syntax
+ | // `MyRule1` over `class:fix.MyRule1`
+ └── scala/fix
+ ├── MyRule1.scala
+ ├── MyRule2.scala
+ └── ...
```

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

```bash
$ sbt
> scalafix MyRule1 MyRule2
```

## Multi-project sbt build

If your build contains several sub-projects, rules can be defined either:

1. within [one of them](#within-a-given-sub-project), which is simpler but
make them impossible to test and complicate sharing rules across
sub-projects;
1. under [their own sub-project(s)](#as-a-separate-sub-project), allowing
usage of [scalafix-testkit](setup.md).

### Within a given sub-project

In a large build with different code owners across sub-projects, it may
be useful to define rules within a given sub-project, without impacting the
global build definition. This helps to iterate on them before potentially
promoting them to the [build-level](#as-a-separate-sub-project).

```diff
repository
├── build.sbt
├── project
│   └── plugins.sbt
└── service1
│   └── src
│   ├── main
│   │   └── ...
│   ├── test
│   │   └── ...
+│   └── scalafix
+│   ├── resources/META-INF/services
+│   │   └── scalafix.v1.Rule // not mandatory, but allows syntax
+| | // `MyRule1` over `class:fix.MyRule1`
+│   └── scala/fix
+│   ├── MyRule1.scala
+│   ├── MyRule2.scala
+│   └── ...
├── service2
│   └── ...
└── service3
   └── ...
```

```diff
// build.sbt
lazy val service1 = project
+ .settings(
+ libraryDependencies +=
+ "ch.epfl.scala" %%
+ "scalafix-core" %
+ _root_.scalafix.sbt.BuildInfo.scalafixVersion %
+ ScalafixConfig
Comment on lines +118 to +122
Copy link
Collaborator Author

@bjaglin bjaglin Jun 21, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@bjaglin bjaglin Jun 22, 2020

Choose a reason for hiding this comment

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

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.

+ )
```

```bash
$ sbt
> service1/scalafix MyRule1 MyRule2
```

### As a separate sub-project

Declaring your rules in a separate sub-project allows you to use
[scalafix-testkit](tutorial.md#write-unit-tests) to easily unit-test your
semantic rules.

```diff
repository
├── build.sbt
├── project
│   └── plugins.sbt
+├── scalafix // same source structure as the scalafix.g8 project template
+│ ├── input/src/main/scala/fix
+│ │   ├── Test1.scala
+│ │   ├── Test2.scala
+│ │   └── ...
+│ ├── output/src/main/scala/fix
+│ │   ├── Test1.scala
+│ │   ├── Test2.scala
+│ │   └── ...
+│ ├── rules/src
Copy link
Collaborator Author

@bjaglin bjaglin Jun 21, 2020

Choose a reason for hiding this comment

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

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.

+│ │   └── main
+│ │     ├── resources/META-INF/services
+│   | │   └── scalafix.v1.Rule // not mandatory, but allows syntax
+| | | // `MyRule1` over `class:fix.MyRule1`
+│ │  └── scala/fix
+│ │  ├── MyRule1.scala
+│ │  ├── MyRule2.scala
+│ │   └── ...
+│ └── tests/src/test/scala/fix
+│    └── RuleSuite.scala
├── service1
│   └── ...
├── service2
│   └── ...
└── service3
   └── ...
```

```diff
// build.sbt
lazy val service1 = project // sub-project where rule(s) will be used
+ .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)
Comment on lines +173 to +205
Copy link
Collaborator Author

@bjaglin bjaglin Jun 21, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

```

```bash
$ sbt
> scalafixTests/test
> service1/scalafix MyRule1 MyRule2
```
1 change: 1 addition & 0 deletions docs/developers/setup.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ plugin. Supported Scala compiler versions include:

- Scala @SCALA211@
- Scala @SCALA212@
- Scala @SCALA213@

The output project can use any Scala compiler version.

Expand Down
4 changes: 4 additions & 0 deletions docs/developers/tutorial.md
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,10 @@ The expansion rules for `github:org/repo` are the following:

## Publish the rule to Maven Central

> If your rule only targets a single codebase and is not meant to be
> distributed, you might want to develop it [as part of your existing
> build](local-rules.md) instead.

Run the following sbt command to publish a rule locally.

```sh
Expand Down
3 changes: 3 additions & 0 deletions website/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
"title": "Contributing Guide",
"sidebar_label": "Guide"
},
"developers/local-rules": {
"title": "Local rules"
},
"developers/patch": {
"title": "Patch"
},
Expand Down
3 changes: 2 additions & 1 deletion website/sidebars.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
"Implementing rules": [
"developers/setup",
"developers/before-you-begin",
"developers/tutorial"
"developers/tutorial",
"developers/local-rules"
],
"API Reference": [
"developers/api",
Expand Down