Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update scalafmt-core to 3.2.2 #4097

Closed
wants to merge 2 commits into from
Closed

Update scalafmt-core to 3.2.2 #4097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fe2be5
Update scalafmt-core to 3.2.2
scala-steward Dec 23, 2021
5267e40
Reformat with scalafmt 3.2.2
scala-steward Dec 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .scalafmt.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version=3.2.1
version=3.2.2
align.openParenCallSite = true
align.openParenDefnSite = true
maxColumn = 120
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/scala/cats/instances/try.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,10 @@ trait TryInstances extends TryInstances1 {
ta match { case Success(a) => Try(map(a)); case Failure(e) => Try(recover(e)) }

override def redeemWith[A, B](ta: Try[A])(recover: Throwable => Try[B], bind: A => Try[B]): Try[B] =
try ta match {
case Success(a) => bind(a); case Failure(e) => recover(e)
} catch { case NonFatal(e) => Failure(e) }
try
ta match {
case Success(a) => bind(a); case Failure(e) => recover(e)
} catch { case NonFatal(e) => Failure(e) }
Comment on lines +78 to +81
Copy link
Contributor

@satorg satorg Dec 23, 2021 •
edited
Loading

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.

Copy link
Contributor

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

Copy link

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.

Copy link
Contributor

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.

Copy link

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?

Copy link

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.

Copy link
Contributor

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.

Copy link

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?

Copy link
Contributor

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?

Copy link
Contributor

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.


override def recover[A](ta: Try[A])(pf: PartialFunction[Throwable, A]): Try[A] =
ta.recover(pf)
Expand Down