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

New scalafmt sbt plugin #1085

Merged
merged 9 commits into from Dec 12, 2017

Conversation

6 participants
@vovapolu
Collaborator

vovapolu commented Nov 30, 2017

#1081

I renamed old plugin to sbt-cli-scalafmt, because it just calls cli interface (and because I couldn't come up with new name for my plugin :D). I also added new scripted tests in addition to old ones.

Plugin doesn't perform any caching, it just looks to unmanagedSources or sbt files and formats them. *Check tasks throw exceptions and print error messages when check is failed.

@vovapolu vovapolu changed the title from New scalafmt plugin to New scalafmt sbt plugin Nov 30, 2017

@sjrd

The *Check tasks should definitely throw exceptions (MessageOnlyExceptions), otherwise they are not usable in the CI since they do not cause a non-zero exit code, hence do not fail the build.

moduleName := "sbt-scalafmt",
isOnly(scala212),
sbtPlugin := true,
sbtVersion in Global := "1.0.0"

This comment has been minimized.

@sjrd

sjrd Nov 30, 2017

Don't put settings in Global in the settings of a project. It is extremely confusing as that affects the whole build, not just the project. It makes no sense.

@sjrd

sjrd Nov 30, 2017

Don't put settings in Global in the settings of a project. It is extremely confusing as that affects the whole build, not just the project. It makes no sense.

This comment has been minimized.

@vovapolu

vovapolu Nov 30, 2017

Collaborator

old plugin has this setting too :)

@vovapolu

vovapolu Nov 30, 2017

Collaborator

old plugin has this setting too :)

This comment has been minimized.

@sjrd

sjrd Nov 30, 2017

Then fix the old plugin too ;)

@sjrd

sjrd Nov 30, 2017

Then fix the old plugin too ;)

This comment has been minimized.

@dwijnand

dwijnand Nov 30, 2017

Contributor

This is effectively defining which sbt version the plugin targets, rather than defaulting whatever sbt version you're using for the whole build as defined in build.properties. I've used this in the past to ensure my plugin works on old versions of sbt while using the latest version to build the plugin.

@dwijnand

dwijnand Nov 30, 2017

Contributor

This is effectively defining which sbt version the plugin targets, rather than defaulting whatever sbt version you're using for the whole build as defined in build.properties. I've used this in the past to ensure my plugin works on old versions of sbt while using the latest version to build the plugin.

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
@olafurpg

Nice work! Thanks a lot @sjrd for the review.

I left some comments.

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Command.args("scalafmt", "run the scalafmt command line interface.") {
case (state, args) =>
org.scalafmt.cli.Cli.main("--non-interactive" +: args.toArray)
PlatformTokenizerCache.megaCache.clear()

This comment has been minimized.

@olafurpg

olafurpg Nov 30, 2017

Member

You want to run this in the new plugin too, megaCache will eventually go away but for now it's best to call it on every file. scalameta/scalameta#1068

@olafurpg

olafurpg Nov 30, 2017

Member

You want to run this in the new plugin too, megaCache will eventually go away but for now it's best to call it on every file. scalameta/scalameta#1068

This comment has been minimized.

@vovapolu

vovapolu Dec 1, 2017

Collaborator

Call on every file? Your plugin calls it once at start.

@vovapolu

vovapolu Dec 1, 2017

Collaborator

Call on every file? Your plugin calls it once at start.

This comment has been minimized.

@vovapolu

vovapolu Dec 1, 2017

Collaborator

And when I should call it? Even when I check source or only when format?

@vovapolu

vovapolu Dec 1, 2017

Collaborator

And when I should call it? Even when I check source or only when format?

This comment has been minimized.

@olafurpg

olafurpg Dec 1, 2017

Member

It's a bit trickier to decide compared to the cli, but I think we can do it once per config/project. This should ideally run whenever scalafmt is not running, otherwise it shoudl be a well-behaving cache and the only penalty for clearing it too often is a very minor slowdown hit.

@olafurpg

olafurpg Dec 1, 2017

Member

It's a bit trickier to decide compared to the cli, but I think we can do it once per config/project. This should ideally run whenever scalafmt is not running, otherwise it shoudl be a well-behaving cache and the only penalty for clearing it too often is a very minor slowdown hit.

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
@vovapolu

This comment has been minimized.

Show comment
Hide comment
@vovapolu

vovapolu Dec 1, 2017

Collaborator

@olafurpg Can you take a look now? I fixed all comments that were unanimously approved :D

Collaborator

vovapolu commented Dec 1, 2017

@olafurpg Can you take a look now? I fixed all comments that were unanimously approved :D

@olafurpg

Changes look great 👍 Second round of review

@@ -0,0 +1,24 @@
> p123/compile:scalafmt

This comment has been minimized.

@olafurpg

olafurpg Dec 2, 2017

Member

Should we maybe add a negative check here?

-> p123/compile:scalafmtCheck

to validate check fails on misformatted files.

@olafurpg

olafurpg Dec 2, 2017

Member

Should we maybe add a negative check here?

-> p123/compile:scalafmtCheck

to validate check fails on misformatted files.

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
@vovapolu

This comment has been minimized.

Show comment
Hide comment
@vovapolu

vovapolu Dec 3, 2017

Collaborator

@olafurpg I think caching isn't going to be easy to implement. Your link is suitable for formatSources task, for example, but not for caching some values computed from files. So I suggest to leave it for next PRs :)

Collaborator

vovapolu commented Dec 3, 2017

@olafurpg I think caching isn't going to be easy to implement. Your link is suitable for formatSources task, for example, but not for caching some values computed from files. So I suggest to leave it for next PRs :)

@olafurpg

Only minor comments remaining, otherwise I think this plugin is soon ready for people to try out! 😄

@vovapolu

This comment has been minimized.

Show comment
Hide comment
@vovapolu

vovapolu Dec 4, 2017

Collaborator

I'm merging it?

Collaborator

vovapolu commented Dec 4, 2017

I'm merging it?

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Dec 4, 2017

Member

Please hold a bit, I want to give the plugin a swing locally first. I was traveling to Munich today, I'll try to take a look tomorrow!

Member

olafurpg commented Dec 4, 2017

Please hold a bit, I want to give the plugin a swing locally first. I was traveling to Munich today, I'll try to take a look tomorrow!

@sjrd

sjrd approved these changes Dec 4, 2017

Looks good from my point of view.

I still think the sbtVersion in Global should be fixed, but it could be in a different PR, given that it's already broken in master anyway because of the other plugin.

@olafurpg

I gave the plugin a swing locally and found a few low hanging improvements

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
},
scalafmtDoFormatOnCompile := Def.settingDyn {
if (scalafmtOnCompile.value) {
scalafmt in resolvedScoped.value.scope

This comment has been minimized.

@olafurpg

olafurpg Dec 5, 2017

Member

This will run scalafmt on all sources on every compile, including nop compiles. It does not take advantage of incrementality. Is this expected/intended behavior? cc/ @fommil I think scalariform uses FileFunction.cached to make this incremental https://github.com/sbt/sbt-scalariform/blob/a378b724a76ee94819939efc0e9adebd896e57d9/src/main/scala/com/typesafe/sbt/Scalariform.scala#L85

@olafurpg

olafurpg Dec 5, 2017

Member

This will run scalafmt on all sources on every compile, including nop compiles. It does not take advantage of incrementality. Is this expected/intended behavior? cc/ @fommil I think scalariform uses FileFunction.cached to make this incremental https://github.com/sbt/sbt-scalariform/blob/a378b724a76ee94819939efc0e9adebd896e57d9/src/main/scala/com/typesafe/sbt/Scalariform.scala#L85

This comment has been minimized.

@fommil

fommil Dec 5, 2017

yeah it's only supposed to be formatting files that are being compiled, not all of them

@fommil

fommil Dec 5, 2017

yeah it's only supposed to be formatting files that are being compiled, not all of them

This comment has been minimized.

@vovapolu

vovapolu Dec 11, 2017

Collaborator

Let's leave caching for next PRs. I would like to implement it entirely, not only in some specific places. I will write a warning in the task description for now :)

@vovapolu

vovapolu Dec 11, 2017

Collaborator

Let's leave caching for next PRs. I would like to implement it entirely, not only in some specific places. I will write a warning in the task description for now :)

This comment has been minimized.

@vovapolu

vovapolu Feb 11, 2018

Collaborator

@olafurpg that's a wrong solution. I don't know why scalariform uses it. FileFunction.cached accepts set of files, if you modify them (and we modify them by formatting), then it thinks that files are changed and runs the function again. So it needs two iteration to "converge" to formatted source and forget about it.

@vovapolu

vovapolu Feb 11, 2018

Collaborator

@olafurpg that's a wrong solution. I don't know why scalariform uses it. FileFunction.cached accepts set of files, if you modify them (and we modify them by formatting), then it thinks that files are changed and runs the function again. So it needs two iteration to "converge" to formatted source and forget about it.

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Dec 5, 2017

Member

I just tried out the plugin on one of my projects and noticed that it formatted files that are excluded with project.excludeFilters in .scalafmt.conf. The plugin should use the (config: ScalafmtConfig).project.matcher.matches(String): Boolean to skip files.

The setting is used here for example: https://github.com/scalameta/language-server/blob/573863cb1db6bf99ac73f35ac17f733b92044beb/.scalafmt.conf#L12-L15

The idiomatic way to do this in sbt-scalafmt would be to customize unmanagedSources.in(scalafmt) but that can't be picked up by editor integrations like IntelliJ, so that's why it's supported via .scalafmt.conf.

Member

olafurpg commented Dec 5, 2017

I just tried out the plugin on one of my projects and noticed that it formatted files that are excluded with project.excludeFilters in .scalafmt.conf. The plugin should use the (config: ScalafmtConfig).project.matcher.matches(String): Boolean to skip files.

The setting is used here for example: https://github.com/scalameta/language-server/blob/573863cb1db6bf99ac73f35ac17f733b92044beb/.scalafmt.conf#L12-L15

The idiomatic way to do this in sbt-scalafmt would be to customize unmanagedSources.in(scalafmt) but that can't be picked up by editor integrations like IntelliJ, so that's why it's supported via .scalafmt.conf.

@olafurpg

Last fixes look great, we can leave caching for another PR. Only minor nitpick on language of logging, then we're good to merge 💯

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
taskKey[Unit]("Format Scala source files if scalafmtOnCompile is on.")
private val scalaConfig =
scalafmtConfig.map(_.map(Config.fromHoconFile(_) match {

This comment has been minimized.

@olafurpg

olafurpg Dec 12, 2017

Member

Could we at least cache this by the timestamp of the file? I feel it's unnecessary to reparse the config for every project/config on every compile.

@olafurpg

olafurpg Dec 12, 2017

Member

Could we at least cache this by the timestamp of the file? I feel it's unnecessary to reparse the config for every project/config on every compile.

This comment has been minimized.

@olafurpg

olafurpg Dec 12, 2017

Member

This comment has been minimized.

@olafurpg

olafurpg Dec 12, 2017

Member

I see, sure we can leave it for a separate PR :)

@olafurpg

olafurpg Dec 12, 2017

Member

I see, sure we can leave it for a separate PR :)

This comment has been minimized.

@vovapolu

vovapolu Feb 9, 2018

Collaborator

Here's what the IntelliJ plugin uses

@olafurpg maybe simply reuse that object? It does just what we need

@vovapolu

vovapolu Feb 9, 2018

Collaborator

Here's what the IntelliJ plugin uses

@olafurpg maybe simply reuse that object? It does just what we need

This comment has been minimized.

@olafurpg

olafurpg Feb 10, 2018

Member

Go ahead, we might want to move it to the core module

@olafurpg

olafurpg Feb 10, 2018

Member

Go ahead, we might want to move it to the core module

Show outdated Hide outdated scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala Outdated
@olafurpg

LGTM 👍 Nice work @vovapolu !

I opened #1091 to track caching

Merging 💃

@olafurpg olafurpg merged commit 3df7c03 into scalameta:master Dec 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Dec 12, 2017

w00t! I'm really looking forward to format on compile in the follow up 😄 sounds like it's currently not doing the right thing?

fommil commented Dec 12, 2017

w00t! I'm really looking forward to format on compile in the follow up 😄 sounds like it's currently not doing the right thing?

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Dec 13, 2017

Member

Format on compile seems to work in the current version but it re-formats all of unmanagedSources.in(scalafmt), regardless of incremental compilation

Member

olafurpg commented Dec 13, 2017

Format on compile seems to work in the current version but it re-formats all of unmanagedSources.in(scalafmt), regardless of incremental compilation

@olafurpg olafurpg added this to the v1.3.1 milestone Jan 1, 2018

@olafurpg olafurpg referenced this pull request Jan 1, 2018

Closed

revamp the sbt plugin #1081

xuwei-k added a commit to xuwei-k/S99 that referenced this pull request Jan 9, 2018

xuwei-k added a commit to dwango/S99 that referenced this pull request Jan 9, 2018

@xuwei-k xuwei-k referenced this pull request Jan 10, 2018

Merged

update sbt plugins #452

@olafurpg olafurpg referenced this pull request May 13, 2018

Closed

autoformat on compile #874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment