add --triggered support when scalafixOnCompile is enabled #169

taisukeoe · 2020-10-10T17:27:47Z

This is a companion Pull Request of scalacenter/scalafix#1217 .

This adds --triggered support when scalafixOnCompile is enabled.

taisukeoe · 2020-10-10T17:58:18Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+      taskKey[Seq[String]](
+        "Pass extra arguments to scalafix only when scalafix is triggred by compilation.  " +
+          "Only --triggered argument is passed by default."
+      )


scalafixArgsOnCompile TaskKey is intended to be more extensible than just passing --triggered.

For example:

scalafixArgsOnCompile += "--check" allows to lint on compilation and rewrite on explicit invocation with the same scalafix configurations.

Preferring TaskKey over SettingKey because TaskKey can rely on other TaskKey values, while scalafixArgsOnCompile is rarely referred by other SettingKeys. (Analogous to scalacOptions)

See my comment in scalacenter/scalafix#1217 (review), I am questionning the need for such a "open" customization point if it's just for --check. Do you have something else in mind?

I'd think several alternatives of arguments on compilation in my mind.

--triggered --check or --triggered : These should work well in most use cases, but it might be confusing if .scalafix.conf gets bigger and a lot of difference in configuration between triggered and explicit invocation.

--config .scalafix.oncompile.conf : In case .scalafix.conf is already big, or overlaying settings will make huge difference between on-compile or not, it would be easier to divide-and-conquer scalafix.conf file rather than overlaying.

--check : This makes scalafixOnCompile just a linter in the simplest way. No triggered overlay. This might be a matter of preference, since this can be the same as --triggerd --check as long as .scalafix.conf has no triggered key.

Disclaimer: these alternatives are based on my assumptions. While I haven't seen HUGE .scalafix.conf which has to be divided and conquered for now, it might be chicken or egg.

I have reconsidered this customization point, and removed it for now.
There are few rules which may break source codes in running on-compile, and can be managed only with --triggered.

taisukeoe · 2020-10-10T18:09:02Z

Question: what is the best way to test sbt-scalafix branch which depends on another wip branch on scalacenter/scalafix?

Currently I change scalafixVersion in project/Dependencies.scala to scalafix SNAPSHOT version which is publish locally. This works in certain scripted tests (in this case scripted sbt-scalafix/scalafixOnCompile), but doesn't work in tests which depend on BuildInfo.scalafixVersion explicitly (e.g. scripted sbt-scalafix/local-rules ).

bjaglin · 2020-10-11T12:57:36Z

Question: what is the best way to test sbt-scalafix branch which depends on another wip branch on scalacenter/scalafix?

Currently I change scalafixVersion in project/Dependencies.scala to scalafix SNAPSHOT version which is publish locally. This works in certain scripted tests (in this case scripted sbt-scalafix/scalafixOnCompile), but doesn't work in tests which depend on BuildInfo.scalafixVersion explicitly (e.g. scripted sbt-scalafix/local-rules ).

There will be a SNAPSHOT published once scalacenter/scalafix#1217 is merged, so you should be able to get the CI green at that point. For now, I guess you could hardcode the SNAPSHOT in

sbt-scalafix/src/sbt-test/sbt-scalafix/local-rules/build.sbt

Line 24 in 8284c35

    
           libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % Versions.scalafixVersion,

as well. We will then hold merging this PR until we bump the to-be-tagged scalafix version on master, at which point I will remove the SNAPSHOTs, merge this, and tag sbt-scalafix.

bjaglin · 2020-10-11T13:11:13Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

@@ -107,7 +113,7 @@ object ScalafixPlugin extends AutoPlugin {
              compile.value // evaluated first, before the potential scalafix evaluation
            if (scalafixOnCompile.value)
              scalafix
-                .toTask("")
+                .toTask(scalafixArgsOnCompile.value.mkString(" ", " ", ""))


it would be nice to update the scripted test about caching, to make sure that an explicit scalafix after a compile with scalafixOnCompile does run rules that are not defined in the triggered overlay.

Given the current cache implementation, I think an explicit scalafix invocation will no longer benefit from the cache of the previous compile invocations after this PR. That's safer, as we can't assume that the configuration is effectively the same and add --triggered in

sbt-scalafix/src/main/scala/scalafix/sbt/ScalafixPlugin.scala

Lines 510 to 511 in 8284c35

case "--check" | "--test" =>

// CHECK & IN_PLACE can share the same cache

. I guess we could do so if we see that there is no triggered section in the configuration.

Thank you for your suggestion. I'll check how cache works later!

--triggered is considered as a cache key only if there is triggered section in .scalafix.conf.

bjaglin · 2021-04-13T10:23:57Z

@taisukeoe I hope you are well! Any update on this? Are you reconsidering the need? (in which case we could deprecate/clean up the parts in the scalafix repo)

taisukeoe · 2021-04-16T06:56:54Z

@bjaglin
Hi, thank you for pinging me. I'll try to finish this PR in this weekend.

taisukeoe · 2021-04-18T14:06:15Z

@bjaglin I'm very sorry for making you wait. Now this PR is ready. Would you please review this when you have time?

bjaglin

Thanks for taking this to the finish line! I really appreciate the extensive testing 🙇 Structure and code LGTM! Please see a few naming/cosmetic suggestions.

Next step would be to document this in the website (https://scalacenter.github.io/scalafix/docs/users/configuration.html looks like a good place?) - unfortunately that's on the scalafix repo!

bjaglin · 2021-04-18T21:07:57Z

src/sbt-test/skip-windows/caching/test

+-> scalafix --triggered --check
+$ delete src/main/scala
+
+# files should not be re-checked in scalafix explicit invocation after scalafix runs onCompile if any triggered configuration is not defined


Suggested change

# files should not be re-checked in scalafix explicit invocation after scalafix runs onCompile if any triggered configuration is not defined

# files should not be re-checked in scalafix explicit invocation after scalafix runs onCompile if no triggered configuration is defined

bjaglin · 2021-04-18T21:09:06Z

src/sbt-test/sbt-scalafix/scalafixOnCompile/build.sbt

@@ -21,3 +21,8 @@ lazy val rewrite = project
    addCompilerPlugin(scalafixSemanticdb)
  )
  .settings(scalafixConfigSettings(IntegrationTest): _*)
+
+lazy val args = project


what does args refer to? what about

Suggested change

lazy val args = project

lazy val triggered = project

bjaglin · 2021-04-18T21:11:00Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

@@ -529,13 +530,29 @@ object ScalafixPlugin extends AutoPlugin {
                // custom tool classpaths might contain directories for which we would need to stamp all files, so
                // just disable caching for now to keep it simple and to be safe
                throw StampingImpossible
+              case "--triggered" =>
+                // If there is a triggered section in a default config file, --triggered flag should be accounted.


looking at the implementation, it seems to work also for non-default configurations?

Suggested change

// If there is a triggered section in a default config file, --triggered flag should be accounted.

// If there is a triggered section in the config file, --triggered flag should be accounted.

bjaglin · 2021-04-18T21:20:24Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+        "Run Scalafix rule(s) declared in .scalafix.conf on compilation and fail on lint errors," +
+          "with overlaying configuration by `triggered` sections if defined in .scalafix.conf file." +


The plural in "sections" confused me a bit. I was using "overlay' as a verb in our previous discussions but I wonder if "overriden" isn't more clear in that context. What about:

Suggested change

"Run Scalafix rule(s) declared in .scalafix.conf on compilation and fail on lint errors," +

"with overlaying configuration by `triggered` sections if defined in .scalafix.conf file." +

"Run Scalafix rule(s) declared in .scalafix.conf on compilation and fail on lint errors. " +

"Default rules & rule configuration can be overriden using the `triggered` section." +

bjaglin · 2021-04-18T21:22:08Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+          val confInArgs = interface.args
+            .collect { case Arg.Config(conf) => conf }
+            .flatten
+            .lastOption


src/sbt-test/skip-windows/caching/files/Triggered.scalafix.conf

taisukeoe · 2021-04-19T03:17:52Z

@bjaglin Thank you for your quick review! I've modified wordings.
As for documentation, is it also worth noting here?
https://scalacenter.github.io/scalafix/docs/users/installation.html#run-scalafix-automatically-on-compile

I think I can finish documentation in this couple of days, by next weekend at the latest. Is it OK for you, or should I hurry a bit more?

taisukeoe · 2021-04-20T15:13:46Z

@bjaglin
I've created a doc PR. Would you please review this? Thanks!
scalacenter/scalafix#1375

taisukeoe commented Oct 10, 2020

View reviewed changes

taisukeoe force-pushed the add-triggered-support branch from d3c1b9b to ed730be Compare October 10, 2020 18:00

bjaglin reviewed Oct 11, 2020

View reviewed changes

Base automatically changed from master to main March 25, 2021 13:52

taisukeoe force-pushed the add-triggered-support branch 2 times, most recently from 2a11644 to b6ee18d Compare April 18, 2021 12:59

add --triggered support to scalafixOnCompile

1407d55

taisukeoe force-pushed the add-triggered-support branch from b6ee18d to 1407d55 Compare April 18, 2021 13:29

taisukeoe marked this pull request as ready for review April 18, 2021 13:29

clean up tests / fix a triggered section check space bug

21288cd

bjaglin reviewed Apr 18, 2021

View reviewed changes

fix some wordings

95e3998

taisukeoe mentioned this pull request Apr 20, 2021

add triggered config description to docs scalacenter/scalafix#1375

Merged

bjaglin approved these changes Apr 20, 2021

View reviewed changes

bjaglin merged commit 6427c80 into scalacenter:main Apr 20, 2021

taisukeoe deleted the add-triggered-support branch April 20, 2021 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --triggered support when scalafixOnCompile is enabled #169

add --triggered support when scalafixOnCompile is enabled #169

taisukeoe commented Oct 10, 2020

taisukeoe Oct 10, 2020 •

edited

Loading

bjaglin Oct 11, 2020

taisukeoe Oct 12, 2020 •

edited

Loading

taisukeoe Apr 18, 2021

taisukeoe commented Oct 10, 2020

bjaglin commented Oct 11, 2020

bjaglin Oct 11, 2020

taisukeoe Oct 12, 2020

taisukeoe Apr 18, 2021

bjaglin commented Apr 13, 2021

taisukeoe commented Apr 16, 2021

taisukeoe commented Apr 18, 2021

bjaglin left a comment

bjaglin Apr 18, 2021

bjaglin Apr 18, 2021

bjaglin Apr 18, 2021

bjaglin Apr 18, 2021

bjaglin Apr 18, 2021

taisukeoe commented Apr 19, 2021 •

edited

Loading

taisukeoe commented Apr 20, 2021

	case "--check" \| "--test" =>
	// CHECK & IN_PLACE can share the same cache

	# files should not be re-checked in scalafix explicit invocation after scalafix runs onCompile if any triggered configuration is not defined
	# files should not be re-checked in scalafix explicit invocation after scalafix runs onCompile if no triggered configuration is defined

	// If there is a triggered section in a default config file, --triggered flag should be accounted.
	// If there is a triggered section in the config file, --triggered flag should be accounted.

		"Run Scalafix rule(s) declared in .scalafix.conf on compilation and fail on lint errors," +
		"with overlaying configuration by `triggered` sections if defined in .scalafix.conf file." +

add --triggered support when scalafixOnCompile is enabled #169

add --triggered support when scalafixOnCompile is enabled #169

Conversation

taisukeoe commented Oct 10, 2020

taisukeoe Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taisukeoe Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taisukeoe commented Oct 10, 2020

bjaglin commented Oct 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin commented Apr 13, 2021

taisukeoe commented Apr 16, 2021

taisukeoe commented Apr 18, 2021

bjaglin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taisukeoe commented Apr 19, 2021 • edited Loading

taisukeoe commented Apr 20, 2021

taisukeoe Oct 10, 2020 •

edited

Loading

taisukeoe Oct 12, 2020 •

edited

Loading

taisukeoe commented Apr 19, 2021 •

edited

Loading