Introduce Scalafmt #2562

joan38 · 2018-10-17T12:28:16Z

This is with mostly the default settings.

I removed some overlapping scalastyle settings.
I raised the scalastyle method line limit from 50 to 55, because I don't see how to decompose those functions like in NonEmptyTraverseTests.nonEmptyTraverse.

The important files are:

project/plugins.sbt
build.sbt
.scalafmt
scalastyle-config.xml

The rest is applied formatting.

Let me know your thoughts.

tpolecat

There's a lot of potential bikeshedding here but I'll limit it to arg lists for round 1. Thanks for taking this on!

tpolecat · 2018-10-17T17:23:20Z

alleycats-laws/src/main/scala/alleycats/laws/discipline/FlatMapRecTests.scala

+                             EqFA: Eq[F[A]]): RuleSet =
+    new DefaultRuleSet(name = "flatMapTailRec",
+                       parent = None,
+                       "tailRecM consistent flatMap" -> forAll(laws.tailRecMConsistentFlatMap[A] _))


I prefer a tighter wrapping style.

def tailRecM[A: Arbitrary]( implicit ArbFA: Arbitrary[F[A]], ArbAFA: Arbitrary[A => F[A]], EqFA: Eq[F[A]] ): RuleSet = new DefaultRuleSet( name = "flatMapTailRec", parent = None, "tailRecM consistent flatMap" -> forAll(laws.tailRecMConsistentFlatMap[A] _) )

It lets you do this if you'd like, you just need to add the returns at the beginning and at the end. I'm not going to go though all of them tho.

codecov-io · 2018-10-17T20:30:09Z

Codecov Report

Merging #2562 into master will decrease coverage by 0.06%.
The diff coverage is 95.16%.

@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   95.21%   95.14%   -0.07%     
==========================================
  Files         361      361              
  Lines        6598     6630      +32     
  Branches      280      289       +9     
==========================================
+ Hits         6282     6308      +26     
- Misses        316      322       +6

Impacted Files	Coverage Δ
...ws/src/main/scala/cats/kernel/laws/OrderLaws.scala	`100% <ø> (ø)`	⬆️
...n/scala/cats/laws/discipline/MonadErrorTests.scala	`100% <ø> (ø)`	⬆️
...ala-2.12-/cats/kernel/compat/TraversableOnce.scala	`0% <ø> (ø)`	⬆️
...main/scala/cats/laws/discipline/FlatMapTests.scala	`100% <ø> (ø)`	⬆️
...src/main/scala/cats/syntax/unorderedFoldable.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/apply.scala	`25% <ø> (ø)`	⬆️
core/src/main/scala/cats/CommutativeApply.scala	`0% <ø> (ø)`	⬆️
.../laws/discipline/CommutativeApplicativeTests.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/InvariantMonoidal.scala	`75% <ø> (ø)`	⬆️
kernel/src/main/scala/cats/kernel/Monoid.scala	`100% <ø> (ø)`	⬆️
... and 284 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 191fae4...c7d2fc3. Read the comment docs.

kailuowang · 2018-10-17T22:40:12Z

so is scalafmt applied on every compile?

joan38 · 2018-10-17T22:45:18Z

@kailuowang No, you need to run sbt fmt.
Intellij Idea picks up the .scalafmt.conf but I wasn't able to load the project in Idea anyway.

ikhoon · 2018-10-18T00:34:36Z

@joan38 I saw some people use vscode, vim and other editors. 😀 neo-sbt-scalafmt automatically reformat code on every compile with some configuration.

kailuowang · 2018-10-18T01:05:40Z

Sorry I missed the previous discussion on this, what did we decide to do IRT enforcing this? add to commit hook? or is there a way to run scalafmt only against changed file (using git)?

ceedubs

Thanks so much @joan38!

I am definitely more concerned about consistency and not having to think about formatting then I am about what the formatting itself is. We'll never be able to find something that everyone is 100% in agreement on.

I left a comment about validateJVM that I think should be addressed in one way or another before we merge this to ensure that in the future CI catches formatting issues.

ceedubs · 2018-10-18T02:30:10Z

build.sbt

+        else
+          Seq(
+            "-Ymacro-annotations"
+          )),


This seems a bit excessive with respect to whitespace to me, but I don't know if this is something that's easily configurable?

It just wants the closing bracket to be on the same indent as the opening one or on a single line.
We just need to put it on a single line the thing. Tho I'm not going to do it for all of them.

ceedubs · 2018-10-18T02:36:08Z

build.sbt

 addCommandAlias("validateFreeJS", "freeJS/test") //separated due to memory constraint on travis
-
-addCommandAlias("validate", ";clean;validateJS;validateKernelJS;validateFreeJS;validateJVM")
+addCommandAlias("validate", ";clean;scalastyle;fmtCheck;validateJS;validateKernelJS;validateFreeJS;validateJVM")


I think that it might be better to move the scalastyle and format check back to validateJVM. While it's not JVM-specific, our build runs validateJVM or validateJS depending on whether it's a JVM or JS build (combining both in a single build leads to more memory issues). So to make sure that this is run, I think that we need to choose one for it to run in, and validateJVM is what we were using.

Our builds probably need a bit of an overhaul, but I don't think that we should try to tackle that in a PR that has this big of a changeset, or we'll never get either merged :)

I reverted this

ceedubs · 2018-10-18T02:39:23Z

core/src/main/scala/cats/Alternative.scala

-   * res1: Option[String] = None
-   * }}}
-   */
+    * Return ().pure[F] if `condition` is true, `empty` otherwise


While we probably haven't been entirely consistent, I think that we've been using javadoc-style comments, so I think that docstrings = JavaDoc might be a good setting for us. Unless I'm wrong and that actually generates a bigger diff...

The diff is much smaller now :)

joan38 · 2018-10-18T07:38:04Z

@ikhoon not only neo-sbt-scalafmt can do it.
Lets decide what you guys want as @kailuowang questionned. Currently you need to run fmt.

ceedubs · 2018-10-18T13:12:35Z

@kailuowang my vote would be to run the scalafmt check as part of validateJVM and have people explicitly call fmt (or whatever) to apply formatting. Doing it on every compile could potentially slow things down or get in the way. But I'm not set in stone on that.

This looks good to me. My only concern is that some builds have been timing out, but I don't know how much that's exacerbated by this PR. I'm checking out a few things locally, but I think that I'd be good with this being merged in its current form.

ceedubs

I just ran this locally, and running fmtCheck took less than a minute, so it shouldn't be contributing too much to builds taking too long.

kailuowang · 2018-10-18T13:23:36Z

I am still a bit concerned that people will keep pushing unformatted code (this new rule is stricter than the previous scalastyle) that fails the build. This may cause more traffic in build queue. I suggest that we add a sbt task that runs some sanity test (like core/test and fmt) before pushing to github. We don't have to have that in this PR, but we should probably have it soon after this is merged.

ceedubs · 2018-10-18T13:40:30Z

@kailuowang yeah, I can see that concern. It's possible that running format on compile would be just fine; I'm not sure. And maybe we could switch things around a bit so validate is something that we would expect people to run before pushing and we'd have a new validateAll that did both JVM and JS (like the current validate does).

BUT since this PR has a huge changeset that's likely to cause merge conflicts, I'm inclined to get this merged and figure that out afterwards.

joan38 · 2018-10-18T14:28:45Z

Thanks guys for your comments. Let me if I can do anything.

kailuowang · 2018-10-18T22:19:39Z

follow up PR #2567

joan38 force-pushed the scalafmt branch 3 times, most recently from 43846cd to 6a802c9 Compare October 17, 2018 16:41

tpolecat reviewed Oct 17, 2018

View reviewed changes

joan38 force-pushed the scalafmt branch 5 times, most recently from d5948b1 to b66681f Compare October 17, 2018 19:13

joan38 force-pushed the scalafmt branch 2 times, most recently from 299b77a to 4cfc2da Compare October 17, 2018 22:25

ceedubs suggested changes Oct 18, 2018

View reviewed changes

Introduce Scalafmt

c7d2fc3

joan38 force-pushed the scalafmt branch from 4cfc2da to c7d2fc3 Compare October 18, 2018 07:42

ceedubs approved these changes Oct 18, 2018

View reviewed changes

kailuowang approved these changes Oct 18, 2018

View reviewed changes

kailuowang merged commit 774fb51 into typelevel:master Oct 18, 2018

kailuowang mentioned this pull request Oct 18, 2018

Implement scalafmt #2546

Closed

joan38 deleted the scalafmt branch October 18, 2018 15:34

kailuowang added this to the 1.5 milestone Oct 30, 2018

kailuowang added the build label Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Scalafmt #2562

Introduce Scalafmt #2562

joan38 commented Oct 17, 2018 •

edited

Loading

tpolecat left a comment

tpolecat Oct 17, 2018

joan38 Oct 17, 2018 •

edited

Loading

codecov-io commented Oct 17, 2018 •

edited

Loading

kailuowang commented Oct 17, 2018

joan38 commented Oct 17, 2018 •

edited

Loading

ikhoon commented Oct 18, 2018 •

edited

Loading

kailuowang commented Oct 18, 2018

ceedubs left a comment

ceedubs Oct 18, 2018

joan38 Oct 18, 2018

ceedubs Oct 18, 2018

joan38 Oct 18, 2018

joan38 Oct 18, 2018

ceedubs Oct 18, 2018

joan38 Oct 18, 2018

joan38 Oct 18, 2018

joan38 commented Oct 18, 2018

ceedubs commented Oct 18, 2018

ceedubs left a comment

kailuowang commented Oct 18, 2018

ceedubs commented Oct 18, 2018

joan38 commented Oct 18, 2018

kailuowang commented Oct 18, 2018

Introduce Scalafmt #2562

Introduce Scalafmt #2562

Conversation

joan38 commented Oct 17, 2018 • edited Loading

tpolecat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joan38 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 17, 2018 • edited Loading

Codecov Report

kailuowang commented Oct 17, 2018

joan38 commented Oct 17, 2018 • edited Loading

ikhoon commented Oct 18, 2018 • edited Loading

kailuowang commented Oct 18, 2018

ceedubs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joan38 commented Oct 18, 2018

ceedubs commented Oct 18, 2018

ceedubs left a comment

Choose a reason for hiding this comment

kailuowang commented Oct 18, 2018

ceedubs commented Oct 18, 2018

joan38 commented Oct 18, 2018

kailuowang commented Oct 18, 2018

joan38 commented Oct 17, 2018 •

edited

Loading

joan38 Oct 17, 2018 •

edited

Loading

codecov-io commented Oct 17, 2018 •

edited

Loading

joan38 commented Oct 17, 2018 •

edited

Loading

ikhoon commented Oct 18, 2018 •

edited

Loading