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.

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

Adds option object ops with catchNonFatal method #3024

Open
wants to merge 4 commits into
base: master
from

Conversation

@stremlenye
Copy link

commented Sep 3, 2019

Introduces the catchNonFatal helper for Option type:

Option.catchNonFatal { "foo".toInt }

It breaks backwards compatibility and I don't really know where to proceed from here.

@ybasket

This comment has been minimized.

Copy link

commented Sep 4, 2019

For the bin compat problem, check how EitherSyntaxBinCompat0 is defined and used, same trick should work here.

Maybe it's also worth to add catchOnly while being on it?

@stremlenye

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

Thanks for the advice.
Regarding catchOnly it's surely doable. I am just not confident enough if its needed in general (even as the member of EitherObjectOps): seems odd to me to wrap the computation into effect and expect it to fail in certain cases.

That's as well leads to cases like this:

type A =type B =type T <: Throwable =type T2 <: Throwable =!= T


// method signature suggests it already takes care of the side effects like exceptions
def foo(a : => A): Either[Throwable, B] =
  Either.catchOnly[T](a).map(…: B)

def a = throw new T2

foo(a) // fails at runtime even though it looked safe from the call site
@ybasket

This comment has been minimized.

Copy link

commented Sep 4, 2019

I agree that your example is something we don't want to see in real code.

But I see some use cases where it can indeed be useful to expect certain exceptions, especially when dealing with Java APIs who declare them to be thrown, for example in number parsing as your initial example:

// using Integer.parseInt to make underlying Java call more visible, 
// .toInt is just syntax for it
Option.catchOnly[NumberFormatException](Integer.parseInt(...))
// vs idiomatic plain Scala - catches everything
Try(Integer.parseInt(...)).toOption
// or cats via Either
Either.catchOnly[NumberFormatException](Integer.parseInt(...)).toOption

The last is how mouse implements its parseDoubleOption, see https://github.com/typelevel/mouse/blob/master/shared/src/main/scala/mouse/string.scala. In these cases, you don't want to catch any exception except NumberFormatException, so a Option.catchOnly would be handy. But this just my 2 cents, just catchNonFatal alone is already something I'd like to use :)

@stremlenye

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

But I see some use cases where it can indeed be useful to expect certain exceptions, especially when dealing with Java APIs who declare them to be thrown

Fair enough. I'll add catchOnly.

@codecov-io

This comment has been minimized.

Copy link

commented Sep 5, 2019

Codecov Report

Merging #3024 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3024      +/-   ##
==========================================
+ Coverage   93.56%   93.57%   +<.01%     
==========================================
  Files         368      368              
  Lines        6949     6955       +6     
  Branches      195      197       +2     
==========================================
+ Hits         6502     6508       +6     
  Misses        447      447
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/option.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 802940c...fa89ef0. Read the comment docs.

@johnynek

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

I'm not crazy about adding 1-off utility methods personally.

An alternative formulation might be adding:

trait ApplicativeError[F[_], E]] {
  def catchNonFatalOr[A](a: => A)(fn: Throwable => F[A]): F[A] =
    try pure(a)
    catch {
      case NonFatal(e) => fn(e)
    }
}

Then this could be ApplicativeError[Option, Unit].catchNonFatalOr(a)(_ => None)

Then catchNonFatal on ApplicativeError could just be: catchNonFatalOr(a)(raiseError(_))

more verbose, but also more general.

@johnynek

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

or, of course Try(a).toOption does the same thing as the Option.catchNonFatal.

@stremlenye

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

@johnynek ApplicativeError already has catchNonFatal method, so the first solution you proposed is already available.

Regarding the second one: that one would do as well but requires instantiation of Try just to convert it into option immediately. Not a huge overhead but completely avoidable.

On the other hand the Option.catchNonFatal provides the uniformal with related data types API and being simply the syntactic sugar aims to rather not to generalise but to provide readable syntax with as less overhead as possible.

@johnynek

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

@stremlenye the first option isn’t available because Option doesn’t implement ApplicativeError for an error type that is a superclass of Throwable. The error type is Unit. So you can’t use the catchNonFatal method with Option currently. That is why I suggested adding a method the converts the Throwable to the Applicative value. Although now that I say it again I realize that could also be in Applicative not Applicative error because it doesn’t use raiseError.

@stremlenye

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

@johnynek I am sorry I am not entirely sure I understood your proposal. Could you please elaborate a bit more on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.