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

Rename Eval.raise to Eval.raiseError #1634

Merged
merged 1 commit into from
May 18, 2017

Conversation

alexandru
Copy link
Member

@alexandru alexandru commented Apr 25, 2017

This PR renames Eval.raise to Eval.raiseError.

This is a pet peeve of mine, but the naming of failing data constructors isn't consistent across the board, so we've got:

  • Eval.raise
  • ApplicativeError.raiseError
  • cats.effect.IO.fail
  • Future.failure
  • scalaz.concurrent.Task.fail
  • scalaz.MonadError.raiseError
  • fs2.Task.fail
  • monix.eval.Task.raiseError

If we have raiseError in ApplicativeError, then I see no reason to not use it across the board.
And if that's not OK for some reason related to Throwable, then the next option should be fail, not raise.

Either way, we should standardise on something.

EDIT: trying to do the same in cats-effect - typelevel/cats-effect#33

@codecov-io
Copy link

codecov-io commented Apr 25, 2017

Codecov Report

Merging #1634 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1634   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         240      240           
  Lines        3937     3937           
  Branches      138      138           
=======================================
  Hits         3676     3676           
  Misses        261      261
Impacted Files Coverage Δ
core/src/main/scala/cats/Eval.scala 97.29% <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 b6fc4e8...657d008. Read the comment docs.

@johnynek
Copy link
Contributor

I removed the labels since these methods were added since the last version. There has been no released version that can be broken.

I'm +1 on naming consistency in general.

@djspiewak
Copy link
Member

I personally prefer the name fail, and raise is certainly shorter than raiseError, but consistency trumps all. Unless we want to actually break compatibility, moving to raiseError is the right way to go.

@milessabin
Copy link
Member

I also prefer fail.

@alexandru
Copy link
Member Author

alexandru commented Apr 26, 2017

OK, bike-shedding time :-))

A more generic debate would be whether we want to use Failure or Error to describe these conditions.

For example here's Roland Kuhn of Akka arguing that we need to differentiate between user input errors, which are to be expected (and recoverable) and failures, which are terminating conditions: https://twitter.com/etorreborre/status/832899323008524290

But MonadError is already established in our community, it's very generic and can also refer to expected input errors, not just non-deterministic Throwables, therefore the naming on MonadError and raiseError is completely fine.

So the two paths that I'm seeing:

  1. should we go ahead with raiseError for all these MonadError types? I think the long name is not that problematic, you get used to it, errors are not thrown lightly anyway so it's actually good that it jumps at you and consistency is really cool because it lowers the barrier for beginners.
  2. should we standardise on having a fail constructor for MonadError[F, Throwable] types?

I would personally prefer to go with raiseError, because at this point it's the path of least resistance.

@djspiewak in regard to your comment, I do think that renaming raiseError to fail on MonadError wouldn't be OK, because error and failure are similar but not equivalent and having fail on MonadError would be odd for people that like to bike-shed on language :-)

Personally I would have preferred just error, because it's a value that we are building there, there's no need for the raise verb. But raiseError is still decent imo. Could have been worse :)

@ceedubs
Copy link
Contributor

ceedubs commented Apr 28, 2017

I give this a 👍 based on a consistency argument. I'm neutral when it comes to error vs fail, but we already have a MonadError type class with raiseError. And if we were to go down the route of calling it fail instead of raiseError there, then it seems to me like the name of the type class should change to MonadFail, and to me that seems like a pretty heavy-handed change for negligible benefit.

@djspiewak
Copy link
Member

Any opinions on raise? I just don't like unnecessarily redundant names. :-)

@johnynek
Copy link
Contributor

I'm +1 on this to get in agreement with MonadError. I hope we won't change MonadError since I view that as a minor change which will cost a lost of migration pain.

@kailuowang kailuowang added this to the 1.0.0-MF milestone Apr 29, 2017
@kailuowang
Copy link
Contributor

If no objection, I am going to merge with 3 sign offs

@kailuowang kailuowang merged commit 2fd530b into typelevel:master 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.

7 participants