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

First stab at a more flexible TransLift #918

Closed

Conversation

djspiewak
Copy link
Member

This is a review PR only! Please do not merge yet. I have some things I'd like to clean up, but I wanted to get some thoughts.

First off, this is an attempted fix for #917, which is a blocker for emm and shims. It rearranges TransLift so that a single instance is no longer fixed to one inner effect type while keeping the flexibility of not over-constraining said effect. For example, the TransLift instance for Kleisli does not constrain the effect in any way, while the OptionT instance requires a Functor and StateT requires Applicative.

Second off… I think I hit a few compiler bugs (def vs val is apparently significant in implicit resolution?!?!) and also may have made the test compilation time even longer. Sorry…

The magic is in TransLift.scala and transLift.scala (the latter is the syntax class and contains some new implicit machinery) I'm very open to nicer ways of doing this, btw, I just don't want to add a second type parameter to the liftT syntax.

BTW, @stew, does this mean I win the race? ;-P

@djspiewak djspiewak force-pushed the feature/translift-expression branch from 31d4d72 to 8dfb9fd Compare March 5, 2016 05:15
@adelbertc
Copy link
Contributor

Must resist urge to merge just so it's in before stew even gets a chance to see it.......

@codecov-io
Copy link

Current coverage is 88.75%

Merging #918 into master will decrease coverage by -0.03% as of 0f6158b

@@            master    #918   diff @@
======================================
  Files          178     178       
  Stmts         2122    2126     +4
  Branches        42      42       
  Methods          0       0       
======================================
+ Hit           1884    1887     +3
  Partial          0       0       
- Missed         238     239     +1

Review entire Coverage Diff as of 0f6158b

Powered by Codecov. Updated on successful CI builds.

new TransLift[OptionT, M] {
def liftT[A](ma: M[A]): OptionT[M, A] = OptionT.liftF(ma)
// do NOT change this to val! I know it looks like it should work, and really I agree, but it doesn't (for... reasons)
implicit def optionTTransLift: TransLift.Aux[OptionT, Functor] =
Copy link
Contributor

Choose a reason for hiding this comment

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

That's really bizarre. In what way does it fail if it's a val?

Copy link
Member Author

Choose a reason for hiding this comment

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

It just fails to resolve for some reason. I didn't look at -Xlog-implicits, but it seemed like it was simply not considering the possibility. Passing it explicitly worked fine, and obviously changing it to a def is also fine.

@ceedubs
Copy link
Contributor

ceedubs commented Mar 8, 2016

I think that this is a more flexible solution than the current implementation. I too would be quite happy to see someone show a cleaner approach, but barring that, I support going forward with something like this.

I'm curious if we could ease the case of lifting a F[_, _] with some helper methods for common cases like Applicative, Functor, and Monad with specialized syntax for those (probably based on Unapply).

@ceedubs
Copy link
Contributor

ceedubs commented Mar 15, 2016

Anyone else have thoughts on this? @stew @tpolecat?

@stew
Copy link
Contributor

stew commented Mar 16, 2016

This LGTM. This is essentially the path I had started heading down, but yeah, djspeiwak wins the race :)

@djspiewak
Copy link
Member Author

Alrighty, if the consensus is that this isn't the worst idea in the world, I'll clean it up and push a full PR.

@adelbertc
Copy link
Contributor

👍

@djspiewak
Copy link
Member Author

/me slaps @adelbertc for not using the github reaction feature

^_^

@djspiewak
Copy link
Member Author

Closing this PR so I can open a new one with clear intent to merge…

@djspiewak djspiewak closed this Mar 20, 2016
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