scalameta / scalafmt

Code formatter for Scala
http://scalameta.org/scalafmt
Apache License 2.0
1.43k stars 277 forks source link

feature request: remove unneeded `{ }` #432

Closed johnynek closed 8 years ago

johnynek commented 8 years ago

I'd love to see code like:

def foo: Int = {
  List(1, 2, 3).sum
}

be rewritten into

def foo: Int = List(1, 2, 3).sum

So, I want an option to only keep = { } if they are required (not a single expression).

olafurpg commented 8 years ago

I agree that this would be a nice feature to have. However, IMO, it belongs best in a separate tool that rewrites code as a pre-processing step before formatting. I need more time to think if we want to run those rewrites as part of scalafmt. The obvious benefit to that approach is that we could leverage the existing scalafmt integrations (sbt, intellij,...).

Ideally, it should be a lot easier to publish plugins that rewrite code and then compose a custom pipeline of rewrites that you want to run as part of your workflow/CI.

Incidentally, I'm working now on scalafix which might be able to accommodate something like that in the future. The top priority for scalafix is Dotty, however.

olafurpg commented 8 years ago

What do you think about limiting this feature to methods that are implemented in a single line?

For example:

// Rewrite
def foo: Int = {
  List(1, 2, 3).sum
}
// Leave alone
def foo: Int = {
  List(1, 2, 3).map { x =>
    x + 1
  }.sum
}
olafurpg commented 8 years ago

Also, what do you think about limiting this rewrite to methods that explicitly provide a non-Unit return types. For example:

// Leave alone
def foo = {
  List(1, 2, 3).sum
}
// Leave alone
def foo: Unit = {
  println(List(1, 2, 3).sum)
}
johnynek commented 8 years ago

I personally would like to enable it regardless of number of lines or Unit return, but some might want a more restrictive setting. Can we not do both?

Personally I find it a good encouragement to write smaller methods that can expressed in a single expression. On Sun, Sep 18, 2016 at 00:36 Ólafur Páll Geirsson notifications@github.com wrote:

Also, what do you think about limiting this rewrite to methods that explicitly provide a non-Unit return types. For example:

// Leave alonedef foo = {

List(1, 2, 3).sum }// Leave alone

def foo: Unit = { println(List(1, 2, 3).sum) }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/olafurpg/scalafmt/issues/432#issuecomment-247840073, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdmm3Dk0Z_qKZ-RVPU2gxkKE44g8fks5qrRQYgaJpZM4J534x .

olafurpg commented 8 years ago

We can make it configurable. Then I would like to fix #316 first.

olafurpg commented 8 years ago

I ended up calling this rule RedundantBraces. It supports these settings: https://github.com/olafurpg/scalafmt/blob/master/core/src/main/scala/org/scalafmt/config/RedundantBracesSettings.scala

Once I release v0.4 (hopefully today, if unstable wifi allows!), I'll add instructions in the docs on how to enable/configure this new feature.

I'm optimistic that we can add other useful syntactic rewrite rules in future releases. All the infrastructure is ready now so contributing new rules should be quite straightforward.

olafurpg commented 8 years ago

I benchmarked the result and there seems to be a ~30% penalty for running all rewrites

    Formatting all of Scala.js (100k LOC):

      default  23.311 ±(99.9%) 13.058 s/op [Average]
      rewrite  29.653 ±(99.9%) 109.585 s/op [Average]

I suspect the performance issue has mostly to do my naive implementation of Patch. We can improve this for future releases.