Enable scalafmt on library/. #2701

sjrd · 2016-12-27T11:00:07Z

No description provided.

Instead of the slightly confusing `@native` and `= native`. To do this we `import scala.scalajs.js` even though we are in that very package.

Only in `library/`.

sjrd · 2016-12-27T11:00:23Z

Review by @gzm0

scala-jenkins · 2016-12-27T12:33:34Z

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/3580/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/4723/
Test PASSed.

gzm0

Big proponent of this!
I'm assuming you want to do this for the whole codebase, but just roll it out incrementally (proponent of that as well :))

gzm0 · 2016-12-29T15:18:57Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

    if (thiz.equals(anotherString)) 0
-    else if ((thiz.asInstanceOf[js.Dynamic] <
-        anotherString.asInstanceOf[js.Dynamic]).asInstanceOf[Boolean]) -1
+    else if ((dynThiz < dynOther).asInstanceOf[Boolean]) -1
    else 1


Did you verify that this can still be inlined the same way?

I haven't verified. I'm quite certain it can, because val dynThiz and val dynOther are pure aliases, which will always be aliased away as LocalDefs.

gzm0 · 2016-12-29T15:20:39Z

library/src/main/scala/scala/scalajs/js/JSNumberOps.scala

@@ -94,16 +94,14 @@ object JSNumberOps {
  @deprecated(
      "A Long is converted to Double to perform JavaScript " +
      "operations. This is almost certainly not what you want. " +
-      "Use `.toDouble` explicitly if you need it.",
-      "0.6.0")
+      "Use `.toDouble` explicitly if you need it.", "0.6.0")


Wrong commit?

gzm0 · 2016-12-29T15:20:44Z

library/src/main/scala/scala/scalajs/js/JSNumberOps.scala

  implicit def enableJSNumberOps(x: Long): JSNumberOps =
    x.toDouble.asInstanceOf[JSNumberOps]

  @deprecated(
      "A Long is converted to Double to perform JavaScript " +
      "operations. This is almost certainly not what you want. " +
-      "Use `.toDouble` explicitly if you need it.",
-      "0.6.0")
+      "Use `.toDouble` explicitly if you need it.", "0.6.0")


Wrong commit?

gzm0 · 2016-12-29T15:22:26Z

library/src/main/scala/scala/scalajs/js/UndefOr.scala

@@ -199,6 +200,7 @@ final class UndefOrOps[A](val self: UndefOr[A]) extends AnyVal {
  @inline final def collect[B](pf: PartialFunction[A, B]): UndefOr[B] =
    if (isEmpty) undefined
    else pf.applyOrElse(this.forceGet, (_: A) => undefined).asInstanceOf[UndefOr[B]]
+  // scalafmt: { maxColumn = 80 }


I would avoid these kind of smaller, local format changes. While for that particular piece of code, it'll probably indeed look nicer, it feels like the maintenance overhead is substantial, for not a lot of gain. (js.Function files are different there IMO).

I made a pretty honest trial at fitting this under 80 characters, and it ended up much much worse. That's why a soft limit is important: it is important to be able to escape the length limit if the alternative is significantly worse.

gzm0 · 2016-12-29T15:22:34Z

library/src/main/scala/scala/scalajs/js/annotation/JSExport.scala

-@field @getter @setter
+@field
+@getter
+@setter


Wrong commit?

gzm0 · 2016-12-29T15:24:57Z

library/src/main/scala/scala/scalajs/runtime/Bits.scala

@@ -80,7 +80,7 @@ object Bits {
   */
  def numberHashCode(value: Double): Int = {
    val iv = rawToInt(value)
-    if (iv == value && 1.0/value != Double.NegativeInfinity) iv
+    if (iv == value && 1.0 / value != Double.NegativeInfinity) iv


Wrong commit?

gzm0 · 2016-12-29T15:25:50Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

@@ -46,7 +46,7 @@ private[runtime] object RuntimeString {

  def codePointAt(thiz: String, index: Int): Int = {
    val high = thiz.charAt(index)
-    if (index+1 < thiz.length) {
+    if (index + 1 < thiz.length) {


Wrong commit?

No this is an undesirable (but tolerated) change. Arithmetic operations on the side of a comparison operator should not have spaces around it, to highlight the relative priority. This is filed as https://github.com/olafurpg/scalafmt/issues/511

gzm0 · 2016-12-29T15:25:52Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

-      NativeJSString.fromCharCode(
-          (offsetCp >> 10) | 0xd800, (offsetCp & 0x3ff) | 0xdc00)
+      NativeJSString
+        .fromCharCode((offsetCp >> 10) | 0xd800, (offsetCp & 0x3ff) | 0xdc00)
    }


Wrong commit?

No, this is an undesirable (but tolerated) change. It should be as before: wrapping the arguments should get higher precedence than a "select chain" if the select chain consists of an only select. This is filed as https://github.com/olafurpg/scalafmt/issues/503

gzm0 · 2016-12-29T15:25:59Z

library/src/main/scala/scala/scalajs/runtime/UndefinedBehaviorError.scala

@@ -19,8 +19,7 @@ class UndefinedBehaviorError(message: String, cause: Throwable)
  def this(cause: Throwable) =
    this(
        "An undefined behavior was detected" +
-        (if (cause == null) "" else ": " + cause.getMessage),
-        cause)
+        (if (cause == null) "" else ": " + cause.getMessage), cause)


Wrong commit?

No, this is an undesirable (but tolerated) change. An argument spanning two lines or more should not share one of its lines with another argument. This is filed as https://github.com/olafurpg/scalafmt/issues/502

gzm0 · 2016-12-29T15:26:05Z

library/src/main/scala/scala/scalajs/runtime/package.scala

@@ -187,7 +187,7 @@ package object runtime {
          if (f0 / twoPowFbits >= 2) {
            //val e = e0 + 1.0 // not used
            val f = 1.0
-            if (e0 > bias-1) { // === (e > bias) because e0 is whole
+            if (e0 > bias - 1) { // === (e > bias) because e0 is whole


Wrong commit?

sjrd

The long-term plan is definitely to enable this on the whole codebase. However, without soft line length limit I am not super confident :s

sjrd · 2016-12-29T22:44:48Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

@@ -46,7 +46,7 @@ private[runtime] object RuntimeString {

  def codePointAt(thiz: String, index: Int): Int = {
    val high = thiz.charAt(index)
-    if (index+1 < thiz.length) {
+    if (index + 1 < thiz.length) {


No this is an undesirable (but tolerated) change. Arithmetic operations on the side of a comparison operator should not have spaces around it, to highlight the relative priority. This is filed as https://github.com/olafurpg/scalafmt/issues/511

sjrd · 2016-12-29T22:45:57Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

-      NativeJSString.fromCharCode(
-          (offsetCp >> 10) | 0xd800, (offsetCp & 0x3ff) | 0xdc00)
+      NativeJSString
+        .fromCharCode((offsetCp >> 10) | 0xd800, (offsetCp & 0x3ff) | 0xdc00)
    }


No, this is an undesirable (but tolerated) change. It should be as before: wrapping the arguments should get higher precedence than a "select chain" if the select chain consists of an only select. This is filed as https://github.com/olafurpg/scalafmt/issues/503

sjrd · 2016-12-29T22:47:05Z

library/src/main/scala/scala/scalajs/runtime/UndefinedBehaviorError.scala

@@ -19,8 +19,7 @@ class UndefinedBehaviorError(message: String, cause: Throwable)
  def this(cause: Throwable) =
    this(
        "An undefined behavior was detected" +
-        (if (cause == null) "" else ": " + cause.getMessage),
-        cause)
+        (if (cause == null) "" else ": " + cause.getMessage), cause)


No, this is an undesirable (but tolerated) change. An argument spanning two lines or more should not share one of its lines with another argument. This is filed as https://github.com/olafurpg/scalafmt/issues/502

sjrd · 2016-12-29T22:47:42Z

library/src/main/scala/scala/scalajs/niocharset/UTF_8.scala

@@ -52,6 +53,7 @@ private[niocharset] object UTF_8 // scalastyle:ignore
      // > 11110111
      -1, -1, -1, -1, -1, -1, -1, -1
  )
+  // format: on


Yes, this is perfectly warranted (hence no comment referencing a scalafmt issue).

sjrd · 2016-12-29T22:49:27Z

library/src/main/scala/scala/scalajs/js/typedarray/package.scala

-      Repr <: TypedArray[T, Repr]](
-      array: Repr, dest: scala.Array[T]): scala.Array[T] = {
+      Repr <: TypedArray[T, Repr]](array: Repr,
+      dest: scala.Array[T]): scala.Array[T] = {


No, this is an undesirable (but barely tolerated) change: if the type argument list spans more than one line, it should not share a line with term parameters. The term parameters should start on their own line. This is filed as https://github.com/olafurpg/scalafmt/issues/504

sjrd · 2016-12-29T22:52:07Z

library/src/main/scala/scala/scalajs/js/typedarray/DataViewExt.scala

      val high = (value >>> 32).toInt
      val low = value.toInt
      dataView.setInt32(index + (if (littleEndian) 4 else 0), high, littleEndian)
-      dataView.setInt32(index + (if (littleEndian) 0 else 4), low,  littleEndian)
+      dataView.setInt32(index + (if (littleEndian) 0 else 4), low, littleEndian)
+      // scalafmt: { maxColumn = 80 }


Yes, the formatting change is in the wrong commit.

The line length limit change is a poor man's implementation of the soft limit at 80 characters. The wrapping to stay under 80 characters is significantly worse than going a few characters over the limit. The soft limit problem is filed as https://github.com/olafurpg/scalafmt/issues/232, although it has been closed as Won't fix.

sjrd · 2016-12-29T22:52:41Z

library/src/main/scala/scala/scalajs/js/annotation/internal/JSOptional.scala

-@field @getter @setter
+@field
+@getter
+@setter


This is an undesirable change. scalafmt 0.5.2 will have an option not to affect the line grouping of annotations, IIUC.

sjrd · 2016-12-29T22:54:20Z

library/src/main/scala/scala/scalajs/js/UndefOr.scala

@@ -199,6 +200,7 @@ final class UndefOrOps[A](val self: UndefOr[A]) extends AnyVal {
  @inline final def collect[B](pf: PartialFunction[A, B]): UndefOr[B] =
    if (isEmpty) undefined
    else pf.applyOrElse(this.forceGet, (_: A) => undefined).asInstanceOf[UndefOr[B]]
+  // scalafmt: { maxColumn = 80 }


I made a pretty honest trial at fitting this under 80 characters, and it ended up much much worse. That's why a soft limit is important: it is important to be able to escape the length limit if the alternative is significantly worse.

sjrd · 2016-12-29T22:56:24Z

library/src/main/scala/scala/scalajs/runtime/RuntimeString.scala

    if (thiz.equals(anotherString)) 0
-    else if ((thiz.asInstanceOf[js.Dynamic] <
-        anotherString.asInstanceOf[js.Dynamic]).asInstanceOf[Boolean]) -1
+    else if ((dynThiz < dynOther).asInstanceOf[Boolean]) -1
    else 1


I haven't verified. I'm quite certain it can, because val dynThiz and val dynOther are pure aliases, which will always be aliased away as LocalDefs.

gzm0 · 2016-12-31T11:23:46Z

undesirable (but tolerated) change.

OK, I see. In that case, can you add this to the commit description so it is clear why formatting changes in two different commits. Also, it might be a good idea to link the issues in the commit.

gzm0 · 2016-12-31T11:24:38Z

And for now, I'm fine with leaving the AdHoc soft-line length settings.

sjrd · 2019-08-29T09:48:06Z

Closing as this would probably have to be redone from scratch at this point.

sjrd added 4 commits December 23, 2016 16:40

Use @js.native and = js.native in the scala.scalajs.js package.

99b1ddb

Instead of the slightly confusing `@native` and `= native`. To do this we `import scala.scalajs.js` even though we are in that very package.

Allow newline (but not space) after [ for Scalastyle.

5d7337c

A bunch of reformattings performed or prompted by scalafmt.

6aadacf

Only in `library/`.

Enable scalafmt on library/.

7e36c3b

sjrd requested a review from gzm0 December 27, 2016 11:00

gzm0 requested changes Dec 29, 2016

View reviewed changes

sjrd commented Dec 29, 2016

View reviewed changes

sjrd closed this Aug 29, 2019

sjrd deleted the scalafmt branch August 29, 2019 09:48

He-Pin mentioned this pull request May 10, 2020

Enable scalafmt on this project #4037

Open

Enable scalafmt on library/. #2701

Enable scalafmt on library/. #2701

Uh oh!

Conversation

sjrd commented Dec 27, 2016

Uh oh!

sjrd commented Dec 27, 2016

Uh oh!

scala-jenkins commented Dec 27, 2016

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzm0 commented Dec 31, 2016

Uh oh!

gzm0 commented Dec 31, 2016

Uh oh!

sjrd commented Aug 29, 2019

Uh oh!

Uh oh!