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

Change MonadError from F[_, _] to F[_] #553

Merged
merged 4 commits into from
Oct 5, 2015

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Oct 1, 2015

I was working with MonadError today and the F[_, _] approach made me use
quite a few type lambdas when I was working with types like Future/Task.
Since the error type is already encoded as another type parameter, as
far as I know we don't really lose anything by using a type constructor
with a single parameter for F.

It's quite possible that there's a reason for using F[_, _]. If anyone
knows of one, please speak up.

I was working with MonadError today and the `F[_, _]` approach made me use
quite a few type lambdas when I was working with types like Future/Task.
Since the error type is already encoded as another type parameter, as
far as I know we don't really lose anything by using a type constructor
with a single parameter for `F`.

It's quite possible that there's a reason for using `F[_, _]`. If anyone
knows of one, please speak up.
@non
Copy link
Contributor

non commented Oct 2, 2015

I don't have a strong opinion of this change. It seems reasonable to me.

If no one speaks up against it then I would say 👍

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 2, 2015

@adelbertc I think you added MonadError. Do you have any thoughts on this? @tpolecat I have heard you chat about error-y type classes like Catchable, MonadError, etc. Do you have thoughts?

@codecov-io
Copy link

Current coverage is 75.76%

Merging #553 into master will decrease coverage by -0.13% as of 3bdaf80

@@            master    #553   diff @@
======================================
  Files          156     156       
  Stmts         2112    2117     +5
  Branches        68      68       
  Methods          0       0       
======================================
+ Hit           1603    1604     +1
  Partial          0       0       
- Missed         509     513     +4

Review entire Coverage Diff as of 3bdaf80

Powered by Codecov. Updated on successful CI builds.

@adelbertc
Copy link
Contributor

Hm, this is a lot nicer and no consequences that I can think of at the moment. 👍 from me, though perhaps we should wait to see if @tpolecat has any concerns before merging?

Also should probably do the same for the other ones, MonadState and MonadReader

@non
Copy link
Contributor

non commented Oct 4, 2015

@ceedubs assuming @tpolecat does sign-off on this, do you want to follow @adelbertc's suggestion and modify MonadState and MonadReader also?

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 4, 2015

@non @adelbertc sure that looks like it would work. I might not get to this for a couple of days, but I'll give it a go.

@ceedubs
Copy link
Contributor Author

ceedubs commented Oct 5, 2015

I updated MonadReader and MonadState.

@adelbertc you mentioned that you were becoming a big fan of these type classes. Do you have a code base using these that you could compile against this branch to see if there might be unfortunate implicit resolution/inference implications? Or are they using Scalaz and not Cats?

@non
Copy link
Contributor

non commented Oct 5, 2015

If @adelbertc signs off on these then 👍 from me too.

@adelbertc
Copy link
Contributor

Looks good to me - the code I have that does use these are all living in Scalaz currently. That being said the tests seem to pick up the instances just fine, so 👍 :-)

adelbertc added a commit that referenced this pull request Oct 5, 2015
Change MonadError from F[_, _] to F[_]
@adelbertc adelbertc merged commit 4fae785 into typelevel:master Oct 5, 2015
@ceedubs ceedubs deleted the monad-error branch November 15, 2015 23:39
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