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

add catchNonFatal to MonadError #1269

Merged
merged 5 commits into from Aug 10, 2016
Merged

Conversation

johnynek
Copy link
Contributor

@johnynek johnynek commented Aug 5, 2016

This method seems really convenient for many common MonadError cases.

It is somewhat limited due to requiring Throwable, we could put it on the MonadError object rather than instance, and we could introduce a new typeclass Catchable[M]?

I think this covers 90% of the cases without adding too much complexity.

@codecov-io
Copy link

codecov-io commented Aug 5, 2016

Current coverage is 90.57% (diff: 100%)

Merging #1269 into master will increase coverage by 0.10%

@@             master      #1269   diff @@
==========================================
  Files           243        243          
  Lines          3286       3301    +15   
  Methods        3234       3243     +9   
  Messages          0          0          
  Branches         49         56     +7   
==========================================
+ Hits           2973       2990    +17   
+ Misses          313        311     -2   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 60ca2b7...2fb162e

* Often E is Throwable. Here we try to call pure or catch
* and raise.
*/
def tryCatch[A](a: => A)(implicit ev: Throwable =:= E): F[A] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about naming this catchNonFatal to match the method on Xor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. Updated.

forAll { e: Either[String, Int] =>
val str = e.fold(identity, _.toString)
val res = MonadError[Try, Throwable].catchNonFatal(str.toInt)
// the above shuold just never cause an uncaught exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/shuold/should

@mpilquist
Copy link
Member

👍

* Often E is Throwable. Here we try to call pure or catch
* and raise.
*/
def catchNonFatal[A](a: => A)(implicit ev: Throwable =:= E): F[A] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first inclination would be to make this eq: Throwable <:< E—that will work in all reasonable places where E is statically known, and it eliminates the need for guesswork about whether to write E =:= Throwable (which won't work) or Throwable =:= E (which will) for people who want to support these methods in generic contexts.

@travisbrown
Copy link
Contributor

👍 but see my question above.

@johnynek johnynek changed the title add tryCatch to MonadError add catchNonFatal to MonadError Aug 9, 2016
@johnynek
Copy link
Contributor Author

this has two 👍 is it okay to merge one's own PR?

* Often E is Throwable. Here we try to call pure or catch
* and raise
*/
def catchNonFatalEval[A](a: Eval[A])(implicit ev: Throwable <:< E): F[A] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this just delegate to catchNonFatal with a.value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be slightly slower since you would allocate another closure to do the call by name.

@adelbertc
Copy link
Contributor

I've done it before, go ahead and merge :)

@johnynek johnynek merged commit 8d5140a into typelevel:master Aug 10, 2016
@stew stew removed the in progress label Aug 10, 2016
@johnynek johnynek deleted the oscar/trypure branch August 10, 2016 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants