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

Make sbt-scalafix follow conventions around sbt configurations #587

Merged
merged 18 commits into from
Feb 12, 2018

Conversation

vovapolu
Copy link
Contributor

@vovapolu vovapolu commented Jan 31, 2018

See #461.
This PR breaks the compatibility with the old sbt-scalafix plugin in order to make sbt-scalafix behave more conventionally. Most critically,

  • scalafix alone runs only in Compile config, test:scalafix must be explicitly triggered to run in Test configuration.
  • inConfig(IntegrationTest)(scalafixConfigSettings) can be used to enable scalafix in the integration test configuration


> compile:scalafixTest ProcedureSyntax
> test:scalafixTest ProcedureSyntax
> it:scalafixTest ProcedureSyntax
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not test that compile:scalafix is independent from test:scalafix

What about:

-> compile:scalafixTest ProcedureSyntax
-> test:scalafixTest ProcedureSyntax
-> it:scalafixTest ProcedureSyntax

> compile:scalafix ProcedureSyntax
> compile:scalafixTest ProcedureSyntax
-> test:scalafixTest ProcedureSyntax
-> it:scalafixTest ProcedureSyntax

> test:scalafix ProcedureSyntax
> test:scalafixTest ProcedureSyntax
-> it:scalafixTest ProcedureSyntax

> it:scalafix ProcedureSyntax
> it:scalafixTest ProcedureSyntax

@MasseGuillaume
Copy link
Contributor

MasseGuillaume commented Jan 31, 2018

Missing: Update website/src/main/tut/docs/users/installation.md

@olafurpg olafurpg added this to the v0.6.0 milestone Jan 31, 2018
@olafurpg
Copy link
Contributor

I'm in favor of breaking stuff in sbt-scalafix and cut a v0.6. Based on feedback, I think this PR is a good improvement.

One more thing I would like to change is stop enabling semanticdb-scalac by default. The auto-installation does not work for all builds, so I prefer to require users to manually addCompilerPlugin(..); scalacOptions += "-Yrangepos" or use scalafixEnable from the shell.

I documented the breaking changes I'd like to make for the next release https://github.com/scalacenter/scalafix/milestone/6

Are there any other breaking changes we want to make?

): Seq[Def.Setting[InputTask[Unit]]] =
(task := impl(configurations).evaluated) +:
configurations.map(c => task.in(c) := impl(Seq(c)).evaluated)
lazy val scalafixConfigSettings: Seq[Def.Setting[_]] = scalafixSettings ++
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to autoImport so users don't have to prefix ScalafixPlugin.?

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Only minor comments on docs, otherwise this is good to go


_Later this plugin has `scalafixConfigure` method to configure scalafix for custom configuration like IntegrationTest.
Since 0.6.0 version it now sticks to more canonical way for such configurations.
The plugin has `scalafixConfigSettings` that can be imported to every configuration by `inConfig(_)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove all updates to the sbt-scalafix installation docs? I will can do it in a followup PR.

A general note, sentences like

The plugin has scalafixConfigSettings that can be imported to every configuration by inConfig(_).

are very helpful if you are not aware of what inConfig(_) does. I generally prefer concrete examples like below since that is useful for both expert and beginner users.

> scalafixCli --rule file:rules/MyRule.scala // Run local custom rule
> scalafixCli --rule github:org/repo/v1 // Run library migration rule
> scalafixCli --test // Make sure no files needs to be fixed
> compile:scalafixCli // Run all rules configured in .scalafix.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the compile: prefix necessary? I would expect it to default to compile:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right :)

@vovapolu
Copy link
Contributor Author

vovapolu commented Feb 6, 2018

I added Olaf format branch here. It should resolve point about [error] gutter from #559.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Any thoughts @MasseGuillaume ?

@MasseGuillaume
Copy link
Contributor

  • Missing: Update website/src/main/tut/docs/users/installation.md
  • What happens with the original task? It's not backward compatible.
  • maybe add a task: scalafixAll to do: it:scalafix, test:scalafix, compile:scalafix

@vovapolu
Copy link
Contributor Author

vovapolu commented Feb 11, 2018

@MasseGuillaume

Missing: Update website/src/main/tut/docs/users/installation.md

I included the updated doc in the previous commit, Olaf said:

Can we remove all updates to the sbt-scalafix installation docs? I will can do it in a followup PR.

What happens with the original task? It's not backward compatible.

Well, yes, that's right :) Let's discuss what we can do with the old tasks.

maybe add a task: scalafixAll to do: it:scalafix, test:scalafix, compile:scalafix

Don't know. It's easy to add, but is it really necessary?

@olafurpg
Copy link
Contributor

@MasseGuillaume docs will need to be updated separately. The next release will be breaking so it's fine to remove the old task.

@olafurpg
Copy link
Contributor

maybe add a task: scalafixAll to do: it:scalafix, test:scalafix, compile:scalafix

This can be left for a separate PR.

@olafurpg olafurpg merged commit 3d4de59 into scalacenter:master Feb 12, 2018
@fommil
Copy link
Contributor

fommil commented Feb 13, 2018

scalafixAll is unnecessary in my opinion... you're assuming a lot about the user's project structure. On larger project with complex test configurations it is normal to set these sorts of things up on a per-project basis. In fact, the ability to run task X for all valid configurations, is a lacking feature in sbt...

@olafurpg olafurpg changed the title Canonical sbt plugin Make sbt-scalafix follow sbt conventions Apr 11, 2018
@olafurpg olafurpg changed the title Make sbt-scalafix follow sbt conventions Make sbt-scalafix follow conventions around sbt configurations Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants