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

Don't infer Null in fromTryCatch #592

Merged
merged 4 commits into from
Nov 7, 2015
Merged

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Oct 31, 2015

This is the start of a fix for #591. So far I've only change Xor, because I wanted to run this approach by people. If people like this approach, then I can also update Validated and try to add some more tests to this PR.

I also renamed fromTryCatch to catching, because it's a bit less verbose and I like the sound of it a little more. I can change it back if people would prefer though.

I also renamed `fromTryCatch` to `catching`, because it's a bit less
verbose and I like the sound of it a little more. I can change it back
if people would prefer though.
@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 31, 2015

By the way, this is what happens now if you omit the type parameter on catching:

scala> Xor.catching(throw new RuntimeException("blargh"))
<console>:14: error: ambiguous implicit values:
 both method ambiguousNull1 in object NotNull of type => cats.NotNull[Null]
 and method ambiguousNull2 in object NotNull of type => cats.NotNull[Null]
 match expected type cats.NotNull[Null]
       Xor.catching(throw new RuntimeException("blargh"))

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 31, 2015

Looking at that error message, I'm a bit tempted to hack around the fact that you can't do something like implicitNotFound for ambiguous implicit values and encode a helpful message in the function names instead of just using ambigrousNull1 and ambiguousNull2 :)

@non
Copy link
Contributor

non commented Oct 31, 2015

Using descriptive method names there sounds smart.

@non
Copy link
Contributor

non commented Oct 31, 2015

Also, I feel like we talked about this before, but how would you feel about a simpler variation that was something like this:

def catching(body: => A): Xor[Throwable, A] =
  try { Right(body) } catch { case e: NonFatal => Left(e) }

Often when working with exceptions I don't care about the type, and the syntax is a lot nicer if you can leave off the annotations.

EDIT: Somehow I completely missed the fact that you defined this as catchingNonFatal. What if we named this something like catchAll instead? (I feel like NonFatal is a reasonable proxy for everything.)

@aryairani
Copy link
Contributor

@ceedubs: you’ve probably seen this, but:

implicitAmbiguous annotation

The @implicitAmbiguous annotation allows customizing the error message when an implicit search finds multiple ambiguous values. Refer to the Scaladoc of the annotation class for an example.

Thanks to @puffnfresh for this contribution!

in scala 2.12.0-M3

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 2, 2015

@refried @puffnfresh I didn't know about @implicitAmbiguous. That's wonderful!

@non is you preference for catchAll because just because it's more concise than catchingNonFatal? I would lean toward being consistent with catching vs catch, and I have a slight leaning toward NonFatal as more descriptive than All, but I don't have particularly strong feelings about this, so I'm fine with changing it.

And also use a slightly ridiculous name for one of the ambiguous
NotNull[Null] instances to steer people in the right direction.
@codecov-io
Copy link

Current coverage is 76.33%

Merging #592 into master will increase coverage by +0.01% as of 1dbec14

@@            master    #592   diff @@
======================================
  Files          160     161     +1
  Stmts         2196    2206    +10
  Branches        68      70     +2
  Methods          0       0       
======================================
+ Hit           1676    1684     +8
  Partial          0       0       
- Missed         520     522     +2

Review entire Coverage Diff as of 1dbec14

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 4, 2015

I've updated my PR. What do people think?

Specifically @non and @mpilquist, since both of you discussed this with me.

Also big thanks to @tpolecat because tut caught several places where I had neglected to update the documentation after these changes :)

@non
Copy link
Contributor

non commented Nov 4, 2015

This seems fine to me. 👍

(I think I'd prefer catchAll but I can live with catchNonFatal -- it reads pretty well in an imperative way: "catch non-fatal exceptions")

@ceedubs
Copy link
Contributor Author

ceedubs commented Nov 5, 2015

@non for what it's worth, this is the conversation that led me to stick with catchNonFatal instead of catchAll. If you feel strongly about this, I'm fine with changing it though.

mpilquist added a commit that referenced this pull request Nov 7, 2015
Don't infer Null in fromTryCatch
@mpilquist mpilquist merged commit dac2335 into typelevel:master Nov 7, 2015
@ceedubs ceedubs deleted the fromTryCatch branch November 15, 2015 23:39
@kailuowang kailuowang mentioned this pull request May 18, 2017
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.

5 participants