Update scalafmt-core to 3.8.6 #1413

scala-steward · 2025-01-24T15:04:53Z

About this PR

📦 Updates org.scalameta:scalafmt-core from 3.8.3 to 3.8.6

Usage

✅ Please merge!

I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.

If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.

Configure Scala Steward for your repository with a .scala-steward.conf file.

Have a fantastic day writing Scala!

⚙ Adjust future updates

Add this to your .scala-steward.conf file to ignore future updates of this dependency:

updates.ignore = [ { groupId = "org.scalameta", artifactId = "scalafmt-core" } ]

Or, add this to slow down future updates of this dependency:

dependencyOverrides = [{
  pullRequests = { frequency = "30 days" },
  dependency = { groupId = "org.scalameta", artifactId = "scalafmt-core" }
}]

^{labels: library-update, early-semver-patch, semver-spec-patch, commit-count:n:3}

stevedlawrence

+1

Do all automated continuous integration checks pass?
Yes, rebased/squashed and rerunning
Is the update a patch, minor, or major update?
Patch update, a number of small reformats but otherwise seems reasonable
Is the license still compatible with ASF License Policy?
Yes, ALv2
Have any changes been made to LICENSE/NOTICE files that need to be incorporated?
Not applicable, we do not distribute
Have any transitive dependencies been added or changed?
Not applicable, we do not distribute

jadams-tresys · 2025-05-07T12:16:15Z

daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala

-          case (_, v :: Nil) :: Nil => { // flag was present with a parameter, convert the parameter
+          case (
+                _,
+                v :: Nil
+              ) :: Nil => { // flag was present with a parameter, convert the parameter


Curious why some lines get expanded like this and others get compressed onto a single line.

I can't really say. I imagine this has to do with long lines and splitting function parameters per line.

We really shouldn't disable scalafmt, so if we don't like these changes, we either need to find and modify a config option that changes it (not sure what that would be or if it would affect other places), open a bug with scalafmt if we think it's wrong, or rewrite it in some way that doesn't require this formatting. Of those, the last is probably the best option, but I'm not sure how much it's wroth it.

I generally let scalafmt do its thing for existing code and just focus on ensuring new code is formatted reasonably.

I took a little look into changing some settings and I think adding

newlines.avoidForSimpleOverflow = [slc]

helps a bit as that will ignore trailing single line comments when determining if we should create a new line. Tried adjusting some settings for XML literals too, but that just made things worse. You can see what the changes would look like here:

main...jadams-tresys:incubator-daffodil:scalafmt

The avoidForSimpleOverflow=[slc] is definitely a reasonable improvement. Loosening line length restrictions in favor a readability is usually a win. That said, in most of the changes I would think just moving the comment to its own line is probably more readable, but I'm not sure it's worth the effort to fix.

jadams-tresys · 2025-05-07T12:16:34Z

daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala

            primType match {
-              case PrimType.Float | PrimType.Double | PrimType.Boolean |
-                  PrimType.HexBinary => /* Non textual data, no need to compare alignment to encoding's expected alignment */
+              case PrimType.Float | PrimType.Double | PrimType.Boolean | PrimType.HexBinary => /* Non textual data, no need to compare alignment to encoding's expected alignment */


This one got compressed onto a single line.

jadams-tresys · 2025-05-07T12:17:27Z

daffodil-core/src/test/scala/org/apache/daffodil/core/dpath/TestDFDLExpressionTree.scala

-      case WholeExpression(_, LiteralExpression(500.0), _, _, _, _) => /* ok */ ;
+      case WholeExpression(_, LiteralExpression(500.0), _, _, _, _) => /* ok */
+        ;


Not really a fan of these changes, is it worth configuring this (if possible)?

jadams-tresys · 2025-05-07T12:17:54Z

daffodil-runtime1/src/test/scala/org/apache/daffodil/runtime1/parser/TestCharsetBehavior.scala

-      intercept[MalformedInputException] { // fails to convert because there is no possible surrogate-pair rep for this.
+      intercept[
+        MalformedInputException
+      ] { // fails to convert because there is no possible surrogate-pair rep for this.


This is just harder to read in my opinion

jadams-tresys · 2025-05-07T12:18:31Z

daffodil-tdml-processor/src/test/scala/org/apache/daffodil/processor/tdml/TestTDMLRunner.scala

-    val testSuite = <testSuite xmlns={tdml} suiteName="theSuiteName">
+    val testSuite = <testSuite xmlns={
+      tdml
+    } suiteName="theSuiteName">


This also seems harder to read.

jadams-tresys

+1

I'm giving this a +1 with my change of adding

newlines.avoidForSimpleOverflow = [slc]

to the settings to ignore single line comments when determining line length for wrapping.

mbeckerle

-1

This change should not be merged. There are places where this changes the meaning and context of comments. This behavior of scalaFmt is a bug and needs fixing.

A line comment isn't necessarily about the last thing on the line. It is about the content
of the line, and adding things to the beginning of that line changes the context of the comment in ways that can break the intent.

mbeckerle · 2025-05-19T16:09:55Z

daffodil-core/src/test/scala/org/apache/daffodil/core/layers/TestLayers.scala

  val le32BitData = Array[Byte](0x01, // BE MSBF
-    0x43, 0x33, 0x33,
-    0x32, // fourbyteswap + LE LSBF (parsed right to left four bytes at a time)
+    0x43, 0x33, 0x33, 0x32, // fourbyteswap + LE LSBF (parsed right to left four bytes at a time)


This kind of upward line recombining is problematic.
In this case, it's ok.

Had that comment been directed specifically at just the line with the 0x32 on it, then this move
would change the meaning of the comment.

If I write this:

class Foo(a1 : Int, a2: Float, // this argument needs some commentary a3: Double ) extends Bar with Baz // for some purpose that requires a bit of explaining

It is unacceptable for it to be turned into:

class Foo(a1 : Int, a2: Float, // this argument needs some commentary a3: Double ) extends Bar with Baz // for some purpose that requires a bit of explaining.

Because that changes the meaning of the comments.

The context of a line comment is the contents of the line at the time the comment is written.
I think if a line has a line comment on that line, that line cannot be moved, or combined with other lines.
You can change the indentation, but you can't merge it upward onto a short line without changing the
context of the comment text.

Ah, I missed that this got changed. 100% agree. I will revert back to the original scala-steward changes.

Executed command: scalafmt --non-interactive

scala-steward mentioned this pull request Jan 24, 2025

Update scalafmt-core to 3.8.5 #1409

Closed

scala-steward force-pushed the update/scalafmt-core-3.8.6 branch from cf53b10 to d883083 Compare April 8, 2025 15:42

stevedlawrence mentioned this pull request May 5, 2025

Update scalafmt-core to 3.8.4 #1405

Closed

stevedlawrence force-pushed the update/scalafmt-core-3.8.6 branch from d883083 to 56054f7 Compare May 5, 2025 14:14

stevedlawrence approved these changes May 5, 2025

View reviewed changes

stevedlawrence added the dependencies Pull requests that update a dependency file label May 5, 2025

jadams-tresys reviewed May 7, 2025

View reviewed changes

jadams-tresys force-pushed the update/scalafmt-core-3.8.6 branch from 56054f7 to 3b0efcb Compare May 19, 2025 16:04

jadams-tresys approved these changes May 19, 2025

View reviewed changes

mbeckerle requested changes May 19, 2025

View reviewed changes

jadams-tresys force-pushed the update/scalafmt-core-3.8.6 branch from 3b0efcb to 56054f7 Compare May 19, 2025 16:40

scala-steward force-pushed the update/scalafmt-core-3.8.6 branch from 56054f7 to 7f47808 Compare July 19, 2025 13:53

scala-steward added 3 commits August 12, 2025 10:44

Update scalafmt-core to 3.8.6

b811634

Reformat with scalafmt 3.8.6

d869e2e

Executed command: scalafmt --non-interactive

Add 'Reformat with scalafmt 3.8.6' to .git-blame-ignore-revs

2a6032a

scala-steward force-pushed the update/scalafmt-core-3.8.6 branch from 7f47808 to 2a6032a Compare August 12, 2025 10:45

Update scalafmt-core to 3.8.6 #1413

Are you sure you want to change the base?

Update scalafmt-core to 3.8.6 #1413

Uh oh!

Conversation

scala-steward commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About this PR

Usage

Uh oh!

stevedlawrence 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!

jadams-tresys left a comment

Choose a reason for hiding this comment

Uh oh!

mbeckerle 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scala-steward commented Jan 24, 2025 •

edited

Loading