Unfortunately, I still consider the lack of decent support for binpacking a serious problem with scalafmt for this codebase. The changes in compiler/ and in OptimizerCore.scala are really bad, for example. Many calls that previously fitted on 2 lines are exploded to 6 lines and more.

We can imagine some progressive adoption through the less critical modules first, but I don't see a path to using scalafmt in the compiler and linker without support for binpacking any time soon.

I totally understand. I think I may try binpacking to see the effect.

If I understand correctly you would like binpacked to remain binpacked and ones that are expanded to stay expanded according to the style guide?

Also, how do you feel about the trailing paren ) being on the next line? Any special rules there?

Thinking about this overall, it would be nice if we could get at least Scala Native and Scala.js to the same format. I will see how it goes and then we can give feedback to the scalafmt folks. They did lots of nice work to fix areas we found in Scala Native.

The rule is actually quite simple:

• If the ) is on a separate line, then it must stay on the separate line and the arguments must be in expanded (non-binpack) mode.
• If the ) is not on its own line, then it must stay on the same line as the last argument, and the arguments must be in binpack mode.

So, when enabling binpack, scalafmt should never decide itself whether an argument list should be in binpack or expanded mode. The decision was made by the human, and indicated by where the ) falls.

Now for dealing with arguments that span several lines themselves in binpack mode, the rule is quite simple as well:

• As soon as an argument spans several lines, it must not share any line with any other argument.

I think (but I may be completely wrong) that the lack of this last rule is what made binpack difficult to implement in scalafmt, all those years ago. With this rule, binpack should not be any more difficult to implement than expanded mode.

This is super helpful - that was my intuition and I also believe that others would probably agree. scalafmt is much more mature now but I will check with the developers to see if that is possible. I will still try some settings to see the results. Some of our generated code in Scala Native could really benefit from this format as well.

I am not sure the binPack.preset is acceptable either but it reduces the diff almost in half.

Diff: diff.txt now 60849 lines in diff

I also filed an issue: scalameta/scalafmt#2633

Now for dealing with arguments that span several lines themselves in binpack mode, the rule is quite simple as well:

• As soon as an argument spans several lines, it must not share any line with any other argument.

@sjrd is this what you expect, then:

  val cls = Select(
    Select(
      Select(
        Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_), scalajs), js),
        nme.annotation, x, y, z),
      internal_, a, b, c),
    wasPublicBeforeTyperXxx)

instead of current

  val cls =
    Select(Select(Select(Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_),
              scalajs), js), nme.annotation, x, y, z), internal_, a, b, c),
      wasPublicBeforeTyperXxx)

Yes, basically. At least in the general case.

There's one potential exception, which I apply as a human, but I could live without it in order to use scalafmt. It is that, if the entire chain can be made to fit on 2 lines by splitting once after an arbitrary comma (even nested), then do that. Otherwise, apply the general case. In the above, for example, if the line length were set 100, it could look like

  val cls = Select(Select(Select(Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_), scalajs),
      js), nme.annotation, x, y, z), internal_, a, b, c), wasPublicBeforeTyperXxx)

but with a line length of 80, it is not possible to fit the whole expression on 2 lines, so it needs to be entirely expanded like you showed.

But again, this is complicated, and I understand that it would be problematic for an auto-formatter to deal with that. So always applying the general rule above would be fine for me.

@ekrich recommend trying 3.0.0-rc7 with the following additional configuration options:

fileOverride {
  "glob:**/scala3/**" {
    runner.dialect = scala3
  }
}

optIn.breakChainOnFirstMethodDot = false

# preserve some overflow
newlines.avoidForSimpleOverflow = [tooLong, punct, slc]

indent.callSite = 4
indentOperator.topLevelOnly = false
indentOperator.include = ".*"
indentOperator.exclude = "^(?:&&|\\|\\||\\+)$"

binPack.parentConstructors = keep

you can also experiment with:

# comment out this line
# newlines.beforeCurlyLambdaParams = multilineWithCaseOnly

newlines.source = keep
binPack.unsafeCallSite = oneline

@kitbellew Thanks for your work and the heads up. I will update the PR and see how it goes.

I updated to the latest version and applied @kitbellew 's recommendations.

The diff went from 76848 to 62813 lines.

diff.txt
I do see some strange indentation. e.g. OptimizerCore.scala line 261 or line 49089 in this diff file.

diff.txt I do see some strange indentation. e.g. OptimizerCore.scala line 261 or line 49089 in this diff file.

4 for case continuation, 4 for tuple ((LongType | ..., ClassType(...))) and 4 for apply (ClassType(...)).

@ekrich i thought this repo had a desire for this: https://scalameta.org/scalafmt/docs/configuration.html#inserting-braces

Updated to latest scalafmt, version 3.3.1. Diff now is 63004 lines vs 62813
diff.txt

Not far down in the diff we start seeing some strange empty comments like /**/. Do we know the purpose of these and/or if there is a remedy?

     /**/
-    /**//***/json.asInstanceOf[JsArray].value(index).asInstanceOf[JsObject].value(fieldName).value
+    /**/
+    /** */

@kitbellew Should this be the case since binpack.literalArgumentLists default is true?

--- a/Users/eric/workspace/scala-js/compiler/src/test/scala/org/scalajs/nscplugin/test/JSGlobalScopeTest.scala
+++ b/Users/eric/workspace/scala-js/compiler/src/test/scala/org/scalajs/nscplugin/test/JSGlobalScopeTest.scala
@@ -331,9 +331,49 @@
     final val ReservedJSIdentifierNames: Set[String] = Set(
-        "arguments", "break", "case", "catch", "class", "const", "continue",
-        "debugger", "default", "delete", "do", "else", "enum", "export",
-        "extends", "false", "finally", "for", "function", "if", "implements",
-        "import", "in", "instanceof", "interface", "let", "new", "null",
-        "package", "private", "protected", "public", "return", "static",
-        "super", "switch", "this", "throw", "true", "try", "typeof", "var",
-        "void", "while", "with", "yield"
+        "arguments",
+        "break",
+        "case",

Maybe some conflicting setting?

The latest snapshot fixes comments like ////// that were being split so we are now back to 62876 line changed.

The wrapping of args in docs also creates quite a bit of extra diff - might look a little better?

+   *  @param args
+   *    the test-framework-specific arguments for the new run

I know we removed the ones after @throws that caused scaladoc not to work.

@kitbellew Should this be the case since binpack.literalArgumentLists default is true?

--- a/Users/eric/workspace/scala-js/compiler/src/test/scala/org/scalajs/nscplugin/test/JSGlobalScopeTest.scala
+++ b/Users/eric/workspace/scala-js/compiler/src/test/scala/org/scalajs/nscplugin/test/JSGlobalScopeTest.scala
@@ -331,9 +331,49 @@
     final val ReservedJSIdentifierNames: Set[String] = Set(
-        "arguments", "break", "case", "catch", "class", "const", "continue",
-        "debugger", "default", "delete", "do", "else", "enum", "export",
-        "extends", "false", "finally", "for", "function", "if", "implements",
-        "import", "in", "instanceof", "interface", "let", "new", "null",
-        "package", "private", "protected", "public", "return", "static",
-        "super", "switch", "this", "throw", "true", "try", "typeof", "var",
-        "void", "while", "with", "yield"
+        "arguments",
+        "break",
+        "case",

Maybe some conflicting setting?

https://scalameta.org/scalafmt/docs/configuration.html#other

binPack.literalsInclude = [
  ".*"
]
binPack.literalsExclude = [
  String
  "Term.Name"
]

The wrapping of args in docs also creates quite a bit of extra diff - might look a little better?

+   *  @param args
+   *    the test-framework-specific arguments for the new run

I know we removed the ones after @throws that caused scaladoc not to work.

1. better how?
2. you could disable scaladoc wrapping, it's not really material

Not far down in the diff we start seeing some strange empty comments like /**/. Do we know the purpose of these and/or if there is a remedy?

They are to automatically insert throw statements to test source maps. The file(s) that have them can definitely be excluded from formatting.

@kitbellew Ok, adding the following helped.

binPack.literalsExclude = []

@gzm0 Yes, that is in the resources directory. Thanks.

Down to 62125 lines now
diff.txt

There is another fix for the non-binpacked array with new BigDecimal(1,2) etc.
scalameta/scalafmt@eb5e513

@sjrd @gzm0, we are getting down to the last few things I have seen that are bad - the diff can also be used to change some spots where wrapping is not optimal. If the review finds some of these I will gladly do some post processing before the final push, after being approved. Please, annotate file and line number. We should use the next release, 3.3.2.

@kitbellew really has been fixing almost everything found so I can't say enough nice things.

First batch of comments.

The wrapping of args in docs also creates quite a bit of extra diff - might look a little better?

+   *  @param args
+   *    the test-framework-specific arguments for the new run

I know we removed the ones after @throws that caused scaladoc not to work.

This looks like a good change, actually, so I'll take it.

This kind of change is annoying, but I understand it's going to be necessary for indentation-based users, so I guess scalafmt won't invest in supporting our previous format:

-      val body = if (targetName.resultTypeRef == VoidRef) {
-        // Materialize an `undefined` result for void methods
-        Block(call, Undefined())
-      } else {
-        call
-      }
+      val body =
+        if (targetName.resultTypeRef == VoidRef) {
+          // Materialize an `undefined` result for void methods
+          Block(call, Undefined())
+        } else {
+          call
+        }

@@ -151,3 +152,3 @@
             if mDef.flags.namespace == MemberNamespace.Public &&
-                mDef.methodName == methodName =>
+              mDef.methodName == methodName =>
           mDef

Maybe this is just a configuration change away, if scalafmt has a configuration for continuation lines of ifs in patterns? Otherwise, I would definitely expect this to follow the general indent.ctrlSite = 4, shouldn't it?

The following is more on the bad side:

@@ -94,4 +95,3 @@
           |console.log("hello");
-          |""".stripMargin,
-        "main.js.map" -> raw"""{
+          |""".stripMargin, "main.js.map" -> raw"""{
           |  "other key": 1

There are quite a few similar changes. These changes actively reduce readability (irrespective of what any style guide could say, IMO).

Can we do anything about that?

Ouch:

@@ -112,12 +112,5 @@
    *
-   *  This assumes the total output is of the following form:
-   *  <no test> ...
-   *  <test 1>
-   *  <no test | test 1> ...
-   *  <test 1>
-   *  <test 2>
-   *  <no test | test 2> ...
-   *  <test 2>
-   *  ...
-   *  <no test> ...
+   *  This assumes the total output is of the following form: <no test> ...
+   *  <test 1> <no test | test 1> ... <test 1> <test 2> <no test | test 2> ...
+   *  <test 2> ... <no test> ...
    */

This one probably requires manual intervention to insert {{{ and }}}. I definitely don't think it's scalafmt's fault here.

Is this expected?

@@ -128,4 +129,4 @@
    *  This applies if either or both of the following are true:
-   *  - It is not a static owner, or
-   *  - It is a non-native JS object.
+   *    - It is not a static owner, or
+   *    - It is a non-native JS object.
    *

If it is, I'll take it.

This one seems plain wrong:

@@ -12,4 +12,5 @@
   assert(fastOptFile.get(scalaJSSourceMap).exists {
-    _.getPath == fastOptFile.data.getPath + ".map"
-  }, "fastOptJS does not have the correct scalaJSSourceMap attribute")
+        _.getPath == fastOptFile.data.getPath + ".map"
+      },
+      "fastOptJS does not have the correct scalaJSSourceMap attribute")
 

Another instance:

@@ -55,8 +57,8 @@
     val t = sum({
-      val y = sum($x1, 7)
-      sum(y, 3) // this will be assigned to a temporary var called `$x1`
-    }, {
-      val z = sum($x1, 12)
-      sum(z, 4)
-    })
+          val y = sum($x1, 7)
+          sum(y, 3) // this will be assigned to a temporary var called `$x1`
+        }, {
+          val z = sum($x1, 12)
+          sum(z, 4)
+        })
     assertEquals(36, t)

I see a lot of undesirable changes in .sbt fields. I suggest excluding .sbt files altogether (most of them are test-only, anyway).

@@ -112,3 +111,4 @@
 
-    def tagAll(internalModuleIDPrefix: String): scala.collection.Map[ClassName, ModuleID] = {
+    def tagAll(internalModuleIDPrefix: String)
+        : scala.collection.Map[ClassName, ModuleID] = {
       tagEntryPoints()

IIRC, at some point scalafmt had a config to never, ever break before the : of a result type. Does that still exist? If yes, we should use it. If not, well, we'll take it.

@@ -593,4 +594,4 @@
        */
-      unsignedDivModHelper(lo, hi, 1000000000, 0,
-          AskToString).asInstanceOf[String]
+      unsignedDivModHelper(lo, hi, 1000000000, 0, AskToString).asInstanceOf[
+          String]
     }

If we manually change it to

      unsignedDivModHelper(lo, hi, 1000000000, 0, AskToString)
        .asInstanceOf[String]

instead, does it stick? Or does scalafmt insist on wrapping inside the [ instead of before the .?

This kind of change is annoying, but I understand it's going to be necessary for indentation-based users, so I guess scalafmt won't invest in supporting our previous format:

-      val body = if (targetName.resultTypeRef == VoidRef) {
-        // Materialize an `undefined` result for void methods
-        Block(call, Undefined())
-      } else {
-        call
-      }
+      val body =
+        if (targetName.resultTypeRef == VoidRef) {
+          // Materialize an `undefined` result for void methods
+          Block(call, Undefined())
+        } else {
+          call
+        }

how would you describe this format? if i am looking at the = and some body following it, how should i tell whether to break and whether to indent?

@@ -151,3 +152,3 @@
             if mDef.flags.namespace == MemberNamespace.Public &&
-                mDef.methodName == methodName =>
+              mDef.methodName == methodName =>
           mDef

Maybe this is just a configuration change away, if scalafmt has a configuration for continuation lines of ifs in patterns? Otherwise, I would definitely expect this to follow the general indent.ctrlSite = 4, shouldn't it?

in case statements, the only indent that applies is caseSite (between case and =>), there's no extra indent for if in that case. the indent you see here is from the infix expression itself. ctrlSite applies to indentation within () if there's a break after the opening one.

The following is more on the bad side:

@@ -94,4 +95,3 @@
           |console.log("hello");
-          |""".stripMargin,
-        "main.js.map" -> raw"""{
+          |""".stripMargin, "main.js.map" -> raw"""{
           |  "other key": 1

I will take a look.

Is this expected?

@@ -128,4 +129,4 @@
    *  This applies if either or both of the following are true:
-   *  - It is not a static owner, or
-   *  - It is a non-native JS object.
+   *    - It is not a static owner, or
+   *    - It is a non-native JS object.
    *

If it is, I'll take it.

Yes. https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html#other-formatting-notes: "you must have extra space in front".

This one seems plain wrong:

@@ -12,4 +12,5 @@
   assert(fastOptFile.get(scalaJSSourceMap).exists {
-    _.getPath == fastOptFile.data.getPath + ".map"
-  }, "fastOptJS does not have the correct scalaJSSourceMap attribute")
+        _.getPath == fastOptFile.data.getPath + ".map"
+      },
+      "fastOptJS does not have the correct scalaJSSourceMap attribute")
 

if i were to describe the approach scalafmt takes is, a multi-line expression should be indented until its last line. in this case, }, "fast..." is not the last line of the assert expression and therefore shouldn't be unindented.

why would you unindent mid-expression, then? how would you describe your indentation philosophy?

how would you make it clear to the reader that these are quite different if indentation is not used:

sum({
  val y = sum($x1, 7)
   sum(y, 3) // this will be assigned to a temporary var called `$x1`
}, sum {
    val z = sum($x1, 12)
    sum(z, 4)
})
sum({
  val y = sum($x1, 7)
   sum(y, 3) // this will be assigned to a temporary var called `$x1`
}), sum({
    val z = sum($x1, 12)
    sum(z, 4)
})

IIRC, at some point scalafmt had a config to never, ever break before the : of a result type. Does that still exist? If yes, we should use it. If not, well, we'll take it.

There's no "never, ever" parameter but there's an "avoid" parameter: newlines.sometimesBeforeColonInMethodReturnType.

Thanks for your reply!

This kind of change is annoying, but I understand it's going to be necessary for indentation-based users, so I guess scalafmt won't invest in supporting our previous format:

-      val body = if (targetName.resultTypeRef == VoidRef) {
-        // Materialize an `undefined` result for void methods
-        Block(call, Undefined())
-      } else {
-        call
-      }
+      val body =
+        if (targetName.resultTypeRef == VoidRef) {
+          // Materialize an `undefined` result for void methods
+          Block(call, Undefined())
+        } else {
+          call
+        }

how would you describe this format? if i am looking at the = and some body following it, how should i tell whether to break and whether to indent?

This is in fact related to

This one seems plain wrong:

@@ -12,4 +12,5 @@
   assert(fastOptFile.get(scalaJSSourceMap).exists {
-    _.getPath == fastOptFile.data.getPath + ".map"
-  }, "fastOptJS does not have the correct scalaJSSourceMap attribute")
+        _.getPath == fastOptFile.data.getPath + ".map"
+      },
+      "fastOptJS does not have the correct scalaJSSourceMap attribute")
 

if i were to describe the approach scalafmt takes is, a multi-line expression should be indented until its last line. in this case, }, "fast..." is not the last line of the assert expression and therefore shouldn't be unindented.

why would you unindent mid-expression, then? how would you describe your indentation philosophy?

The answer to both lives in a single rule of indentation policy:

When we find a {, assuming we can fit everything until that { on the current line, we look at the indentation of the start of its line, call it N. Go the next line and indent the content by N+2. Put the closing } at indentation N. Then, after the }, pretend that everything from { to } was a single atomic expression, which logically fit on one line, without interrupting the line flow of the line that contained the {.

When there are lambda parameters (param1, param2) =>, they are put on the same line as { if possible. Otherwise they are put on the next line at N+2, and the rest of the block is then at N+4. The rest stays the same.

This philosophy explains why

      val body = if (targetName.resultTypeRef == VoidRef) {
        // Materialize an `undefined` result for void methods
        Block(call, Undefined())
      } else {
        call
      }

is formatted like that. Everything until the { fits on one line. Then everything until } is one atomic expression that pretends to still be on that line. So else is still on the same line and keeps the flow, and hencethe second { ... } follows the same rule.

It also explains

  assert(fastOptFile.get(scalaJSSourceMap).exists {
    _.getPath == fastOptFile.data.getPath + ".map"
  }, "fastOptJS does not have the correct scalaJSSourceMap attribute")

with the same reasoning.

It is also with the same rule that stuff like the following is formatted as is:

  val foo = xs.map { x =>
    x + 1
  }

which I believe is quite common. In fact, I don't know how to justify that without some reasoning similar to the rule I use. If we applied the same philosophy that seems to be applied to

  assert(fastOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fastOptFile.data.getPath + ".map"
      },
      "fastOptJS does not have the correct scalaJSSourceMap attribute")

I would expect it to be formatted as

  val foo = xs.map { x =>
      x + 1
    }

@@ -151,3 +152,3 @@
             if mDef.flags.namespace == MemberNamespace.Public &&
-                mDef.methodName == methodName =>
+              mDef.methodName == methodName =>
           mDef

Maybe this is just a configuration change away, if scalafmt has a configuration for continuation lines of ifs in patterns? Otherwise, I would definitely expect this to follow the general indent.ctrlSite = 4, shouldn't it?

in case statements, the only indent that applies is caseSite (between case and =>), there's no extra indent for if in that case. the indent you see here is from the infix expression itself. ctrlSite applies to indentation within () if there's a break after the opening one.

Hum, I guess that makes sense. I'll take it.

Yes. https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html#other-formatting-notes: "you must have extra space in front".

Ah, well, 1 point for scalafmt 😅

Thanks for your reply!

The answer to both lives in a single rule of indentation policy:

When we find a {, assuming we can fit everything until that { on the current line, we look at the indentation of the start of its line, call it N. Go the next line and indent the content by N+2. Put the closing } at indentation N. Then, after the }, pretend that everything from { to } was a single atomic expression, which logically fit on one line, without interrupting the line flow of the line that contained the {.

the problem is, unfortunately, that it might lead to the so-called state explosion in scalafmt. at the beginning of every argument (which is way before your {), it needs to decide whether to break or continue (so two choices already); now you are saying that there should be a third choice: break; continue as one line, without indentation, ignoring {}; or continue with indentation. might be very hard.

  assert(fastOptFile.get(scalaJSSourceMap).exists {
    _.getPath == fastOptFile.data.getPath + ".map"
  }, "fastOptJS does not have the correct scalaJSSourceMap attribute")

a few months ago (probably way above in this same conversation), you pointed out that if an argument is multiline, it cannot be on the same line as another argument. yet in this case, the first argument to assert is multiline but you are not expecting a break after the comma and before the second argument. perhaps i misunderstood your original requirement.

  val foo = xs.map { x =>
    x + 1
  }

which I believe is quite common. In fact, I don't know how to justify that without some reasoning similar to the rule I use.

i already mentioned it earlier, indentation stops before the last line. single-arg calls sometimes behave differently.

a few months ago (probably way above in this same conversation), you pointed out that if an argument is multiline, it cannot be on the same line as another argument. yet in this case, the first argument to assert is multiline but you are not expecting a break after the comma and before the second argument. perhaps i misunderstood your original requirement.

Indeed, that is the rule. You are perfectly right.

It doesn't contradict the above because, when we break a {}, it pretends to have always been a single line. It's like the } is still on the same line as {, for the purposes of all the other rules.

the problem is, unfortunately, that it might lead to the so-called state explosion in scalafmt. at the beginning of every argument (which is way before your {), it needs to decide whether to break or continue (so two choices already); now you are saying that there should be a third choice: break; continue as one line, without indentation, ignoring {}; or continue with indentation. might be very hard.

I don't think so. Locally there are still two choices: break or continue. When later it sees a {, it immediately validates everything it has seen so far on the logical line; it can commit to it, because it will always have to break right after the { (or after a lambda param list, but if that doesn't work out it falls back on breaking after the { anyway). And whatever comes after the { cannot have any impact on what was before it.

If anything, I believe this strategy allows the formatter to commit to a particular choice sooner rather than later.

I know this rule is unusual. Although it has worked really well for us, I understand that I can't really expect scalafmt to invest in it. If we can't get this to work, at least we should format

  assert(fastOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fastOptFile.data.getPath + ".map"
      }, "fastOptJS does not have the correct scalaJSSourceMap attribute")

as

  assert(
      fastOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fastOptFile.data.getPath + ".map"
      },
      "fastOptJS does not have the correct scalaJSSourceMap attribute")

instead, as per the rule that you already mentioned:

a few months ago (probably way above in this same conversation), you pointed out that if an argument is multiline, it cannot be on the same line as another argument.

but which also applies wrt. (. A multi-line argument cannot be on the same line as the (, since that causes weird issues like this, whether {} are involved or not.

a few months ago (probably way above in this same conversation), you pointed out that if an argument is multiline, it cannot be on the same line as another argument. yet in this case, the first argument to assert is multiline but you are not expecting a break after the comma and before the second argument. perhaps i misunderstood your original requirement.

Indeed, that is the rule. You are perfectly right.

It doesn't contradict the above because, when we break a {}, it pretends to have always been a single line. It's like the } is still on the same line as {, for the purposes of all the other rules.

if you are saying that breaks in {} do not make the argument multiline, that may have been a good thing to mention earlier. as it stands, and based on your requirements as stated originally, the binpack=oneline feature has been released with a more conventional definition of multiline, and therefore this logic can't be modified, at least not without annoying someone who is already using it.

the problem is, unfortunately, that it might lead to the so-called state explosion in scalafmt. at the beginning of every argument (which is way before your {), it needs to decide whether to break or continue (so two choices already); now you are saying that there should be a third choice: break; continue as one line, without indentation, ignoring {}; or continue with indentation. might be very hard.

I don't think so. Locally there are still two choices: break or continue. When later it sees a {, it immediately validates everything it has seen so far on the logical line; it can commit to it, because it will always have to break right after the { (or after a lambda param list, but if that doesn't work out it falls back on breaking after the { anyway). And whatever comes after the { cannot have any impact on what was before it.

If anything, I believe this strategy allows the formatter to commit to a particular choice sooner rather than later.

perhaps. i have fiddled with trying to optimize the formatter algorithm but it's tricky because it is very likely to introduce formatting differences in some other cases.

if you are saying that breaks in {} do not make the argument multiline, that may have been a good thing to mention earlier. as it stands, and based on your requirements as stated originally, the binpack=oneline feature has been released with a more conventional definition of multiline, and therefore this logic can't be modified, at least not without annoying someone who is already using it.

I'm pretty sure I explained this rule several times over the decade of development of scalafmt, including to Ólaf when he was sitting in my office back then. 😄 But it's also very likely that I failed to mention it in the critical places where this information would have been found. So 🤷‍♂️ I guess it's on me. ;)

I guess I didn't mention it recently, and especially wrt. binpacking, because for us this is orthogonal to binpacking. If it were linked to binpacking, it would not explain the if or the map cases.

As I said above, I understand that this is probably out of scope.

I must also say that the support for binpacking itself is actually quite remarkable now. It seems to be doing exactly the right thing everywhere! So thanks a lot for that!

Now, which is much improved vs 62125.

% wc -l diff.txt                     
   54741 diff.txt

diff.txt

Seems a little non-deterministic but I cleaned up the config file a bit and the line count went down.

% wc -l diff.txt                     
   54662 diff.txt

@sjrd you want all the .sbt files excluded?

@sjrd you want all the .sbt files excluded?

Yes, I think so. As well as the .scala files in project/, which are basically .sbt files.

@sjrd The project directory is already excluded.

I looked at that case in the sbt file you were looking at and I think either format after scalafmt seems ok.

check := {
  val fastOptFile = (fastOptJS in Test).value
  assert(fastOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fastOptFile.data.getPath + ".map"
      },
      "fastOptJS does not have the correct scalaJSSourceMap attribute")

  val fullOptFile = (fullOptJS in Test).value
  assert(
      fullOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fullOptFile.data.getPath + ".map"
      },
      "fullOptJS does not have the correct scalaJSSourceMap attribute")
}

There only about 5 embedded .sbt files in the diff with only some small adjustments to indent. I think the diff was hard to visualize.

Update: here is the new diff

Is the intent to remove the include and exclude with the new setting? Edit: Albert answered and said no.

indentOperator.exemptScope = aloneArgOrBody
indentOperator.include = ".*"
indentOperator.exclude = "^(?:&&|\\|\\||\\+)$"

55655 lines in diff
diff.txt

Updated to 3.4.3.

New diff has 56062 lines, more than the previous version.

diff2.txt

If there are places we should hand tweak - we can capture those and fix in a followup. Also, if you have generated files we can avoid formatting those in a comment.

Will need the following in .gitignore

scripts/.coursier
scripts/.scalafmt*

As a reminder that I haven't forgotten this is hanging out here, we could create .git-blame-ignore-revs.

# scalafmt initial commit
825ba00bb95c0a9bb801ccb4c6dd74ec7b59b59e

# Some other mass formatting
a6aff6bf8da21a4e6377238ae95b3c9bb941d655

The rule is actually quite simple:

• If the ) is on a separate line, then it must stay on the separate line and the arguments must be in expanded (non-binpack) mode.
• If the ) is not on its own line, then it must stay on the same line as the last argument, and the arguments must be in binpack mode.

So, when enabling binpack, scalafmt should never decide itself whether an argument list should be in binpack or expanded mode. The decision was made by the human, and indicated by where the ) falls.

@sjrd can you please clarify whether this rule is to apply to method calls only, and not to method definitions? currently, scalafmt handles method definitions differently, ignoring any break before the closing parenthesis.

p.s. would it be useful to describe this in https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md#method-call-style and possibly just below in https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md#method-definition, rather than (or in addition to) https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md#indentation?

@sjrd can you please clarify whether this rule is to apply to method calls only, and not to method definitions? currently, scalafmt handles method definitions differently, ignoring any break before the closing parenthesis.

Yes, the same rule would be applied to definitions in my book. But it might be two separate config options for other people?

@sjrd thanks for the reply on method definitions. a comment and a question below.

I know this rule is unusual. Although it has worked really well for us, I understand that I can't really expect scalafmt to invest in it. If we can't get this to work, at least we should format

  assert(fastOptFile.get(scalaJSSourceMap).exists {
        _.getPath == fastOptFile.data.getPath + ".map"
      }, "fastOptJS does not have the correct scalaJSSourceMap attribute")

i am trying to add another binpacking option, calling it SjsOneline, which excludes these blocks when looking for newlines in multi-line and removes the extra indentation.

a few months ago (probably way above in this same conversation), you pointed out that if an argument is multiline, it cannot be on the same line as another argument.

but which also applies wrt. (. A multi-line argument cannot be on the same line as the (, since that causes weird issues like this, whether {} are involved or not.

sorry to continue asking about this. can you please tell me how you foresee the following formatted:

  foo(arg1_short, arg2_very_long(arg21, arg22_very_very_very_very_long))(curried_arg3, ...)
  foo(arg1_short, arg2_very_long(arg21, arg22_very_very_very_very_long)).select(more_args...)
  foo(arg1_short, arg2_very_long(arg21, // comment forcing multi-line
    arg22))(curried_arg3, ...)
  foo(arg1_short, arg2_very_long("""|multi-line
    |string
    |""".stripMargin)).select(more_args...)

should it be

  // multi-line arg from one argument clause cannot be on the same line as arg from that or another arg clause
  foo(arg1_short,
    arg2_very_long(arg21,
    arg22_very_very_very_very_long))(
    curried_arg3, ...)
  foo(arg1_short,
    arg2_very_long(arg21,
    arg22_very_very_very_very_long))
    .select(more_args...)
  // embedded comment is considered multi-line
  foo(arg1_short,
    arg2_very_long(arg21, // comment forcing multi-line
    arg22))(
    curried_arg3, ...)
  // multi-line string is multi-line
  foo(arg1_short,
    arg2_very_long("""|multi-line
    |string
    |""".stripMargin))
    .select(more_args...)

or

  // multi-line arg from one argument clause can be on the same line as arg from another arg clause
  foo(arg1_short,
    arg2_very_long(arg21,
    arg22_very_very_very_very_long))(curried_arg3, ...)
  foo(arg1_short,
    arg2_very_long(arg21,
    arg22_very_very_very_very_long)).select(more_args...)
  // embedded comment is not considered multi-line
  foo(arg1_short,
    arg2_very_long(arg21, // comment forcing multi-line
    arg22))(curried_arg3, ...)
  // multi-line string is not multi-line, just like blocks
  foo(arg1_short,
    arg2_very_long("""|multi-line
    |string
    |""".stripMargin)).select(more_args...)

Worksheet I was using for analysis - probably could be way better ways to do this but at least this could be a starting point for someone. https://gist.github.com/ekrich/3cf859046a013d1b74a1be05383657d4