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 ApplicativeThrow and MonadThrow to cats package object (from cats-effect) #3696

Merged
merged 2 commits into from
Nov 26, 2020

Conversation

agustafson
Copy link
Contributor

@agustafson agustafson commented Nov 26, 2020

I noticed that the ApplicativeThrow and MonadThrow type aliases had been added to cats-effect:
https://github.com/typelevel/cats-effect/blob/series/2.x/core/shared/src/main/scala/cats/effect/package.scala#L41-L43

I felt that it made more sense for it to be cats as ApplicativeError and MonadError are in cats rather than cats-effect.

Also, it seems like ApplicativeThrow has erroneously had an unused A type param added, so I figured it was best to remove it. I'm not sure if that will clash or cause problems if both cats._ and cats.effect._ are imported. I'll open a separate PR against cats-effect 2.x branch to remove the A

This is a kind reminder to run sbt +prePR and commit the changed files, if any, before submitting.

As an aside, I tried running this task but it failed on master as well as my PR, but the code all seems to be formatted correctly.

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #3696 (d9b8c79) into master (ba70c8c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3696   +/-   ##
=======================================
  Coverage   90.23%   90.23%           
=======================================
  Files         391      391           
  Lines        8861     8861           
  Branches      242      242           
=======================================
  Hits         7996     7996           
  Misses        865      865           

@agustafson
Copy link
Contributor Author

It'd also be kinda nice if we could add a

object MonadThrow {
  def apply[F[_]](implicit ev: MonadThrow[F]): MonadThrow[F] = ev
}

but wasn't sure where we might put that

@bplommer
Copy link
Contributor

If/when this is released I'd suggest we remove these from CE3 and change the ones in CE2 to be aliases for these.

@bplommer
Copy link
Contributor

It'd also be kinda nice if we could add a

object MonadThrow {

  def apply[F[_]](implicit ev: MonadThrow[F]): MonadThrow[F] = ev

}

but wasn't sure where we might put that

Good idea - I think it could either go directly below the alias or in a package cats {} block in the same file directly below the package object

@agustafson
Copy link
Contributor Author

It'd also be kinda nice if we could add a

object MonadThrow {

  def apply[F[_]](implicit ev: MonadThrow[F]): MonadThrow[F] = ev

}

but wasn't sure where we might put that

Good idea - I think it could either go directly below the alias or in a package cats {} block in the same file directly below the package object

Yeah, I wondered if putting it in the same file as ApplicativeError/MonadError made sense as an alternative. Just so that the package object didn't get too crowded.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

I like it 👍

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

I like this a lot. So much better than having it in Cats Effect

@djspiewak djspiewak merged commit cf72378 into typelevel:master Nov 26, 2020
@agustafson agustafson deleted the move-MonadThrow branch November 27, 2020 10:17
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