[SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 #26655

srowen · 2019-11-25T01:29:49Z

What changes were proposed in this pull request?

Move scalafmt to Scala 2.12 profile; bump to 0.12.

Why are the changes needed?

To facilitate a future Scala 2.13 build.

Does this PR introduce any user-facing change?

None.

How was this patch tested?

This isn't covered by tests, it's a convenience for contributors.

SparkQA · 2019-11-25T01:33:20Z

Test build #114363 has finished for PR 26655 at commit 05265bc.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-11-25T01:53:46Z

The plugin changes the parameters used to skip formatting, so it failed because it tried to run scalafmt. That is easy enough to fix, both in fixing the parameter and also moving to pluginManagement; it shouldn't actually be used by default by the build.

But, this highlights another problem: scalafmt / plugin doesn't seem to work on the complex Spark code at this point: https://scalameta.org/scalafmt/docs/known-issues.html#deeply-nested-code . It will fail if you run it with the script.

So I'm also wondering whether we just yank this.

srowen · 2019-11-25T01:55:58Z

pom.xml

@@ -166,7 +166,6 @@
    <commons.collections.version>3.2.2</commons.collections.version>
    <scala.version>2.12.10</scala.version>
    <scala.binary.version>2.12</scala.binary.version>
-    <scalafmt.parameters>--diff --test</scalafmt.parameters>


I think this is the problem. parameters is no longer available in the plugin. Without it, I think it formats all files and fails. I don't see a way to have the plugin not do this, hm.

Oh, that sounds like a serious regression of scalafmt.

Well, the plugin more than scalafmt. We could pursue a workaround, but maybe not worth it; I'm not sure how much this is used.

Really, the current state of this PR isn't great, as it no longer does anything useful. I'll need to either delete it or revert and move it behind a Scala 2.12 profile. Any opinion?

I'm in favor of trying to keep this functionality, since "nit: 4 spaces" is not a welcoming first experience for new contributors.

It looks like the plugin stopped using the cli interface to scalafmt, which is why parameters aren't accepted anymore. I've opened SimonJPegg/mvn_scalafmt#48 to see if the plugin author has any suggestions.

srowen · 2019-11-25T01:56:32Z

Another option is to leave this available only for the 2.12 build and revert this change, but I'm wondering if it's worth keeping anyway. Do devs use it?

SparkQA · 2019-11-25T03:46:00Z

Test build #114366 has finished for PR 26655 at commit f13f51e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

koeninger · 2019-11-25T16:30:49Z

pom.xml

@@ -166,7 +166,6 @@
    <commons.collections.version>3.2.2</commons.collections.version>
    <scala.version>2.12.10</scala.version>
    <scala.binary.version>2.12</scala.binary.version>
-    <scalafmt.parameters>--diff --test</scalafmt.parameters>


I'm in favor of trying to keep this functionality, since "nit: 4 spaces" is not a welcoming first experience for new contributors.

It looks like the plugin stopped using the cli interface to scalafmt, which is why parameters aren't accepted anymore. I've opened SimonJPegg/mvn_scalafmt#48 to see if the plugin author has any suggestions.

koeninger · 2019-11-25T16:31:21Z

dev/scalafmt

-params="${@:---diff}"
-
-./build/mvn mvn-scalafmt_2.12:format -Dscalafmt.skip=false -Dscalafmt.parameters="$params"
+./build/mvn mvn-scalafmt_2.12:format -Dscalafmt.skip=false


Even if we figure out what's up with parameters, this would need to be either scalafmt_2.12 or scalafmt_2.13 accordingly

Yeah, right, noticed that one too. I will instead change this to put the older version of the plugin under the scala-2.12 profile, to get it out of the way of the 2.13 build, for now. If the plugin further updates, we can use this approach here.

…behavior

srowen · 2019-11-25T19:17:33Z

I updated to just:

Move scalafmt under the 2.12 profile
Move it to pluginManagement for good measure (still works)
Add the extra skip configs that version 1.x wants, before I forget
Manually tested that it does reformat, and only files in the current changelist

SparkQA · 2019-11-25T21:20:29Z

Test build #114422 has finished for PR 26655 at commit 9142562.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-11-26T10:01:07Z

Retest this please

pom.xml

SparkQA · 2019-11-26T12:21:46Z

Test build #114456 has finished for PR 26655 at commit 9142562.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you, @srowen and @koeninger .
I also checked this is still active with Scala-2.12 also 0.12 becomes more verbose.

[INFO] Using config at path: dev/.scalafmt.conf
[INFO] Options: --diff
[ERROR] Could not locate Scala source at /Users/dongjoon/PRS/PR-26655/external/kafka-0-10-assembly/src/main/scala
[ERROR] Could not locate Scala source at /Users/dongjoon/PRS/PR-26655/external/kafka-0-10-assembly/src/test/scala
[ERROR] No source files found, skipping formatting!

### What changes were proposed in this pull request? Move scalafmt to Scala 2.12 profile; bump to 0.12. ### Why are the changes needed? To facilitate a future Scala 2.13 build. ### Does this PR introduce any user-facing change? None. ### How was this patch tested? This isn't covered by tests, it's a convenience for contributors. Closes apache#26655 from srowen/SPARK-29293. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Update scalafmt for compatibility with Scala 2.13

05265bc

srowen requested a review from koeninger November 25, 2019 01:29

srowen self-assigned this Nov 25, 2019

Update scalafmt plugin args, move to pluginManagement

f13f51e

srowen commented Nov 25, 2019

View reviewed changes

dongjoon-hyun added the BUILD label Nov 25, 2019

koeninger reviewed Nov 25, 2019

View reviewed changes

Instead, move scalafmt under 2.12 profile for now, and keep existing …

9142562

…behavior

dongjoon-hyun reviewed Nov 26, 2019

View reviewed changes

pom.xml Show resolved Hide resolved

srowen changed the title ~~[SPARK-29293][BUILD] Update scalafmt plugin to 1.0.2 for compatibility with Scala 2.13~~ [SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 Nov 26, 2019

dongjoon-hyun approved these changes Nov 26, 2019

View reviewed changes

dongjoon-hyun closed this in e23c135 Nov 26, 2019

srowen deleted the SPARK-29293 branch December 6, 2019 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 #26655

[SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 #26655

srowen commented Nov 25, 2019 •

edited by dongjoon-hyun

Loading

SparkQA commented Nov 25, 2019

srowen commented Nov 25, 2019

srowen Nov 25, 2019

dongjoon-hyun Nov 25, 2019

srowen Nov 25, 2019

srowen Nov 25, 2019

koeninger Nov 25, 2019

srowen commented Nov 25, 2019

SparkQA commented Nov 25, 2019

koeninger Nov 25, 2019

koeninger Nov 25, 2019

srowen Nov 25, 2019

srowen commented Nov 25, 2019

SparkQA commented Nov 25, 2019

dongjoon-hyun commented Nov 26, 2019

SparkQA commented Nov 26, 2019

dongjoon-hyun left a comment •

edited

Loading

[SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 #26655

[SPARK-29293][BUILD] Move scalafmt to Scala 2.12 profile; bump to 0.12 #26655

Conversation

srowen commented Nov 25, 2019 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Nov 25, 2019

srowen commented Nov 25, 2019

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

srowen commented Nov 25, 2019

SparkQA commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srowen commented Nov 25, 2019

SparkQA commented Nov 25, 2019

dongjoon-hyun commented Nov 26, 2019

SparkQA commented Nov 26, 2019

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

srowen commented Nov 25, 2019 •

edited by dongjoon-hyun

Loading

dongjoon-hyun left a comment •

edited

Loading