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

Adds a MonadTrans typeclass #844

Closed
wants to merge 5 commits into from
Closed

Adds a MonadTrans typeclass #844

wants to merge 5 commits into from

Conversation

stew
Copy link
Contributor

@stew stew commented Jan 31, 2016

This is similar to MonadTrans in scalaz. It allows you to lift a Monad into a transformer wrapping that monad

I skipped Hoist, since I've never seen that actually used in the wild.

I was shamed into doing this by @djspiewak :)

@codecov-io
Copy link

Current coverage is 89.12%

Merging #844 into master will decrease coverage by -0.14% as of ece069f

@@            master    #844   diff @@
======================================
  Files          168     170     +2
  Stmts         2319    2336    +17
  Branches        75      75       
  Methods          0       0       
======================================
+ Hit           2070    2082    +12
  Partial          0       0       
- Missed         249     254     +5

Review entire Coverage Diff as of ece069f

Powered by Codecov. Updated on successful CI builds.

}

object MonadTrans {
def apply[MT[_[_], _]](implicit MT: MonadTrans[MT]) = MT
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing return type

@ceedubs
Copy link
Contributor

ceedubs commented Feb 1, 2016

Looks good. I just left a couple very minor comments.

I think we could also have instances for Kleisli and StateT, though that could be a separate PR.


implicit val optionTMonadTrans: MonadTrans[OptionT] = new MonadTrans[OptionT] {
def liftM[M[_], A](ma: M[A])(implicit M: Monad[M]): OptionT[M, A] =
OptionT(M.map(ma)(Some.apply))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could just delegate to liftF

@ceedubs
Copy link
Contributor

ceedubs commented Feb 1, 2016

I think that MonadTrans is really helpful. However, there is one thing that bothers me a little bit about it. It seems overly-specific to Monad. Most of these transformers actually have constraints that are weaker than Monad in order to lift into them. For example, we can lift any F[A] into a Kleisli without constraints on F. We can lift any F[A] where F has a Functor into an OptionT.

I'm pondering whether it might make sense to have something like Transformer[F[_], T[_[_], _]] that also abstracts over what the type class needed by the constructor is. I might be trying to get a little too creative, and it might run into all sorts of inference issues. But I'd kind of like to give it a try. Any thoughts?

@ceedubs
Copy link
Contributor

ceedubs commented Feb 1, 2016

Hmm I'm confused by this. Your branch seems to not have the latest from master, so Cartesian is still called Monoidal. Yet you've added CartesianSyntax and somehow the build passed?

@travisbrown
Copy link
Contributor

@ceedubs Travis CI will use GitHub's merge commit, which in this case is ece069f, which has 7c9c209 for a parent and includes the Cartesian change. (I thought there was some way to have the merge commit automatically update for new commits on the target branch, but I might have been making that up.)

@ceedubs
Copy link
Contributor

ceedubs commented Feb 1, 2016

Thanks @travisbrown. It works fine if I merge in the latest from master.

I tried out my idea and I think it looks like it could work. I'll probably submit a PR (maybe to Stew's branch) tomorrow for discussion.

@aryairani
Copy link
Contributor

thanks @stew, this helped me finally understand MonadTrans.

@stew
Copy link
Contributor Author

stew commented Feb 2, 2016

OMG @ceedubs YES, I have it. I'm closing this PR in favor of #853 which generalizes this to not always need monads to lift

@stew stew closed this Feb 2, 2016
@pakoito pakoito mentioned this pull request May 31, 2017
54 tasks
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