add --triggered flag to load overlay section in .scalafix.conf #1217

taisukeoe · 2020-07-19T15:32:28Z

This is a follow up PullRequest of scalacenter/sbt-scalafix#148 .

As discussed at the previous PullRequest, we find it would be better to support on-compile specific scalafix configuration in CLI, rather than sbt-scalafix.

So that I would add --on-compile flag and support onCompile section in .scalafix.conf .

sbt-scalafix follow up

With this PullRequest, sbt-scalafix can pass --on-compile as a scalafix InputTask argument. So scalafix running on-compile will respect onCompile configuration in .scalafix.conf.

And I have an extra idea:

What do you think about adding scalafixArgumentOnCompile: SettingKey[String] (or SettingKey[Seq[String]]) key to sbt-scalafix?
With scalafixArgumentOnCompile := "--check", users can switch their rules to lint only in their every scalafix run on-compile, which might be useful.

support onCompile section in .scalafix.conf

taisukeoe · 2020-07-22T08:52:38Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

+  // will be overridden by
+  //   onCompile = {
+  //     DisableSyntax.noThrows = true
+  //   }


Or should we allow to override DisableSyntax.noThrows only,
and keep DisableSyntax.noVars = true or other DisableSyntax settings?

I'm wondering what is the best... 🤔
Your feedback is much appreciated.

I thought merging-shallow strategy is simpler and clearer, since users can understand what is going to run without merging Confs one by one in their mind.

However, I'd imagine several concrete use cases -- for example, users just want to turn OrganizeImports.removeUnused into false on compile. With merging-shallow strategy, they might face severe conf duplications especially in OrganizeImports.groups (or it can be generalized as Conf.Lsts), which are not pleasant.

So I have changed my mind and implementation with 342f74d .
Now Confs are merged deeply, so that users can add onCompile.OrganizeImports.removeUnused = false with keep respecting original OrganizeImports.groups Conf.

I'd still want to hear your opinions about this.

bjaglin · 2020-07-23T07:06:52Z

Thanks for following up @taisukeoe - it's vacation time here, so expect some delays for the review

taisukeoe · 2020-07-23T07:47:08Z

@bjaglin Thank you for your response.
No problem, please enjoy your vacation 👍

bjaglin

Big apologies for getting back so late to this PR! Thanks for following up after our long discussion in scalacenter/sbt-scalafix#148, let's get this (and the sbt-scalafix companion to come) in before the next release!

I have a few implementation cosmetic comments, but mostly I am wondering if onCompile is not too specific as a build tool might invoke this in a different context than compile. From your use-case, I feel like you want a specific set of rules and/or rule settings when scalafix is triggered "in the background" (which can be "sneaky" for the user). Would --non-interactive / nonInteractive be a more generic name for that?

bjaglin · 2020-10-02T20:32:09Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

+      "Run only rules defined in onCompile section in .scalafix.conf," +
+        "or rules explicitly passed via --rules"


Maybe this is a good place to mention the fact that the section only overlays the default stuff?

Suggested change

"Run only rules defined in onCompile section in .scalafix.conf," +

"or rules explicitly passed via --rules"

"Overlay the default rules & rule settings in .scalafix.conf with the `onCompile` section"

bjaglin · 2020-10-02T20:33:11Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

@@ -242,9 +247,21 @@ case class Args(
    RuleDecoder.decoder(ruleDecoderSettings.withConfig(scalafixConfig))
  }

+  // With a --on-compile flag, looking for settings in onCompile block first, and fallback to standard settings.
+  def preProcessedConf(base: Conf): Conf =


if we use the overlay term, this could be more precise

Suggested change

def preProcessedConf(base: Conf): Conf =

def maybeOnCompileOverlaidConf(base: Conf): Conf =

Overlay seems to be a nicer word 👍

bjaglin · 2020-10-02T20:34:29Z

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

+      val confOnCompile = ConfGet.getKey(base, "onCompile" :: Nil)
+      confOnCompile.fold(base)(
+        ConfOps.merge(ScalafixConfOps.drop(base, "onCompile"), _)
+      )


while you are at it, can't you extract the entire get/fold/merge logic to ScalafixConfOps so we better understand the intent here?

taisukeoe · 2020-10-06T16:38:07Z

@bjaglin Thank you for your reviewing!
Indeed on-compile would be too specific, and it would be nicer to be relevant name for a build tool or an editor.

While I'd agree that --non-interactive can be applied to more general use case than --on-compile, I'm wondering how we can make it more straight forward name. (In general, it would be nicer to rephrase non-XXX to YYY).

Considering the nature that it adds a hook on compile, what do you think about --hooked or --triggered?

I'm happy to apply suggested changes to this branch, once we fix the term.
I'm planning to do this stuff, let's say this weekend. Is it OK for your release schedule?

bjaglin

While I'd agree that --non-interactive can be applied to more general use case than --on-compile, I'm wondering how we can make it more straight forward name. (In general, it would be nicer to rephrase non-XXX to YYY).

Considering the nature that it adds a hook on compile, what do you think about --hooked or --triggered?

I would say triggered is the best proposal so far - I went through the PR and added suggestions in a few places to see how it looks. @mlachkar your opinion is welcome on this as I guess this can be useful for the Metals integration.

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixMainMode.java

scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixArguments.java

scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CliSyntacticSuite.scala

…MainMode.java Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

…Arguments.java Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

…cticSuite.scala Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

taisukeoe · 2020-10-10T13:45:28Z

I've reflected review comments with triggered.
I'm still fine to change the term triggered if better proposal comes before merging.

bjaglin

👍 LGTM!

@mlachkar any objection/opinion?

What do you think about adding scalafixArgumentOnCompile: SettingKey[String] (or SettingKey[Seq[String]]) key to sbt-scalafix?
With scalafixArgumentOnCompile := "--check", users can switch their rules to lint only in their every scalafix run on-compile, which might be useful.

For that, we could have a scalafixCheckOnCompile setting key (mutually exclusive with scalafixOnCompile), which would request a CHECK_TRIGGERED mode execution I guess? Having an arbitrary list of arguments means that we wouldn't fully control the mode, so it can be confusing to know which configuration (overlay) is used, no?

taisukeoe · 2020-10-12T16:59:14Z

Thank you for reviewing again!

For that, we could have a scalafixCheckOnCompile setting key (mutually exclusive with scalafixOnCompile), which would request a CHECK_TRIGGERED mode execution I guess?

Since we can just use both of CHECK and IN_PLACE_TRIGGERED at once, I wouldn't think we need to introduce new CHECK_TRIGGERED mode. I also worry that exclusiveness between different sbt key seems to be tricky.

Having an arbitrary list of arguments means that we wouldn't fully control the mode, so it can be confusing to know which configuration (overlay) is used, no?

I see your point.
If we have another mode to overlay config than triggered in future, it will be confusing.

Or, related to tool integration, is there any reason to clarify or restrict which arguments are passed on compile?

bjaglin · 2020-10-22T21:58:05Z

@olafurpg any objection?

taisukeoe added 2 commits July 20, 2020 00:18

add --on-compile flag

a49c525

support onCompile section in .scalafix.conf

fix errors in ci-211

4310eca

taisukeoe mentioned this pull request Jul 20, 2020

add scalafixOnCompileConfig to separate .scalafix.conf for scalafix-on-compilation scalacenter/sbt-scalafix#148

Closed

taisukeoe commented Jul 22, 2020

View reviewed changes

merge Confs for onCompile deeply

342f74d

bjaglin mentioned this pull request Oct 2, 2020

document scalafixOnCompileConfig key & scalafixOnCompile shortcomings #1211

Closed

bjaglin reviewed Oct 2, 2020

View reviewed changes

bjaglin reviewed Oct 6, 2020

View reviewed changes

taisukeoe and others added 8 commits October 10, 2020 15:35

Update scalafix-interfaces/src/main/java/scalafix/interfaces/Scalafix…

a4782be

…MainMode.java Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

Update scalafix-interfaces/src/main/java/scalafix/interfaces/Scalafix…

c5d0825

…Arguments.java Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

Update scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala

93f48a2

Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

Update scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CliSynta…

cd423e5

…cticSuite.scala Co-authored-by: Brice Jaglin <bjaglin@gmail.com>

rename --on-compile to --triggered

79a36f3

extract conf overlay logic to ScalafixConfOps

04f7cd2

merged master

05c3435

fix imports

ac0e1c3

taisukeoe mentioned this pull request Oct 10, 2020

add --triggered support when scalafixOnCompile is enabled scalacenter/sbt-scalafix#169

Merged

bjaglin approved these changes Oct 11, 2020

View reviewed changes

bjaglin changed the title ~~add --on-compile flag / support onCompile section in .scalafix.conf~~ add --triggered flag to load overlay section in .scalafix.conf Oct 11, 2020

bjaglin merged commit 37a29f4 into scalacenter:master Nov 1, 2020

taisukeoe mentioned this pull request Apr 20, 2021

add triggered config description to docs #1375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --triggered flag to load overlay section in .scalafix.conf #1217

add --triggered flag to load overlay section in .scalafix.conf #1217

taisukeoe commented Jul 19, 2020 •

edited

Loading

taisukeoe Jul 22, 2020 •

edited

Loading

taisukeoe Jul 24, 2020 •

edited

Loading

bjaglin commented Jul 23, 2020

taisukeoe commented Jul 23, 2020

bjaglin left a comment

bjaglin Oct 2, 2020

bjaglin Oct 2, 2020

taisukeoe Oct 10, 2020

bjaglin Oct 2, 2020

taisukeoe commented Oct 6, 2020

bjaglin left a comment •

edited

Loading

taisukeoe commented Oct 10, 2020 •

edited

Loading

bjaglin left a comment •

edited

Loading

taisukeoe commented Oct 12, 2020 •

edited

Loading

bjaglin commented Oct 22, 2020

		"Run only rules defined in onCompile section in .scalafix.conf," +
		"or rules explicitly passed via --rules"

	"Run only rules defined in onCompile section in .scalafix.conf," +
	"or rules explicitly passed via --rules"
	"Overlay the default rules & rule settings in .scalafix.conf with the `onCompile` section"

	def preProcessedConf(base: Conf): Conf =
	def maybeOnCompileOverlaidConf(base: Conf): Conf =

add --triggered flag to load overlay section in .scalafix.conf #1217

add --triggered flag to load overlay section in .scalafix.conf #1217

Conversation

taisukeoe commented Jul 19, 2020 • edited Loading

sbt-scalafix follow up

taisukeoe Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

taisukeoe Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

bjaglin commented Jul 23, 2020

taisukeoe commented Jul 23, 2020

bjaglin left a comment

Choose a reason for hiding this comment

bjaglin Oct 2, 2020

Choose a reason for hiding this comment

bjaglin Oct 2, 2020

Choose a reason for hiding this comment

taisukeoe Oct 10, 2020

Choose a reason for hiding this comment

bjaglin Oct 2, 2020

Choose a reason for hiding this comment

taisukeoe commented Oct 6, 2020

bjaglin left a comment • edited Loading

Choose a reason for hiding this comment

taisukeoe commented Oct 10, 2020 • edited Loading

bjaglin left a comment • edited Loading

Choose a reason for hiding this comment

taisukeoe commented Oct 12, 2020 • edited Loading

bjaglin commented Oct 22, 2020

taisukeoe commented Jul 19, 2020 •

edited

Loading

taisukeoe Jul 22, 2020 •

edited

Loading

taisukeoe Jul 24, 2020 •

edited

Loading

bjaglin left a comment •

edited

Loading

taisukeoe commented Oct 10, 2020 •

edited

Loading

bjaglin left a comment •

edited

Loading

taisukeoe commented Oct 12, 2020 •

edited

Loading