-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update scalafmt-core to 3.2.2 #4097
Update scalafmt-core to 3.2.2 #4097
Conversation
try | ||
ta match { | ||
case Success(a) => bind(a); case Failure(e) => recover(e) | ||
} catch { case NonFatal(e) => Failure(e) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if scalafmt says "A" why does not it say "B" making it formatted in this way:
try
ta match {
case Success(a) => bind(a); case Failure(e) => recover(e)
}
catch { case NonFatal(e) => Failure(e) }
Because to me the current reformatting looks more weird and hard-to-read than it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked about it in the scalafmt channel:
https://discord.com/channels/632642981228314653/632665341864181780/923692567370022962
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that is the correct format inline with braceless syntax. Interesting that a minor point release would do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not seem correct to me. In fact, when I first saw the change, I got surprised because I didn't find the catch
block for the try
right away. Because the way how scalafmt formats it may now create an impression that catch
belongs to somewhere else deep inside rather than the outermost try
. Similar to:
if (checkCondition())
ta match {
case Success(a) => bind(a); case Failure(e) => recover(e)
} else doSomethingOtherwise()
looks quite confusing but
if (checkCondition())
ta match {
case Success(a) => bind(a); case Failure(e) => recover(e)
}
else doSomethingOtherwise()
looks way clearer and easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was looking at the try
where it wrapped ta match {
. what happens if you wrap the catch
to the next line, does it stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 3.3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kitbellew personally I don't want to configure it. I just want it to have a good standard style even if it isn't my first choice and I want that configuration to not require updates. Instead my CIs have been going red because updating scalafmt has recently changed either the formatting or the configuration file or both. I appreciate the work people have given on the project, I just wish it were more conservative so that unless you get a major version bump your previous config will be valid and the format won't change (unless you opt in to new versions).
Anyway this is OT for here but indulged myself. Apologies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if nothing changes... why do you need to upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I wonder – if it supposed to be fixed in version 3.3.0 (which just has been released) then should we just skip this one and close this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kitbellew we upgrade because scala-steward sends the update and there may be bug fixes or security fixes. Those are the motivations I think most libraries have for updating.
Let me close this PR – seems we agreed that the way this version reformats the code is not quite pretty. |
Updates org.scalameta:scalafmt-core from 3.2.1 to 3.2.2.
GitHub Release Notes - Version Diff
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!
Ignore future updates
Add this to your
.scala-steward.conf
file to ignore future updates of this dependency:labels: library-update, early-semver-patch, semver-spec-patch, commit-count:n:2