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

More flexible TransLift #940

Merged
merged 6 commits into from
Apr 3, 2016

Conversation

djspiewak
Copy link
Member

Reimplements TransLift to no longer fix the type constructor. This is substantially more flexible and makes it possible to achieve the same generality as scalaz's Hoist. This is done while retaining the flexibility of cats's previous implementation, which is that liftT requires the minimal constraint necessary for the target MT. In most cases, this is Functor, but in some cases (like Kleisli) it is nothing at all (i.e. no constraint).

To handle the "nothing at all" case, I have added a Trivial type, which is basically the unit typeclass. It takes no parameters and has a single instance which is guaranteed to be in the implicit scope. Several type aliases are provided to allow Trivial to be used in slots of varying shapes. A F[_] typeclass (e.g. Monoid) can be satisfied by Trivial.P1, and a F[_[_]] typeclass (e.g. Monad) can be satisfied by Trivial.PH1. I'm very open to nicer naming.

Oh, and for students of SI-2712, the way that TLExtract is encoded (specifically with SingletonMT and SingletonM) demonstrates an interesting workaround to some of its foibles. One concern here is that the error messages will be horrendous. But without control over the type toString in @implicitNotFound, we can't really do better.

This PR addresses #917.

@djspiewak
Copy link
Member Author

Er… what? The build is hanging? This change definitely increases the compilation time of TransLiftTests, but it's not increased by that much. Furthermore, the hang appears to be happening at runtime. I did confirm that TransLiftTests runs to completion locally, so that can't be it. Anyone have any idea what's going on?

@DavidGregory084
Copy link
Member

@djspiewak yeah, this seems to be a big problem at the minute, see the discussion on #938, #810, #942 about fixing this

@djspiewak
Copy link
Member Author

@DavidGregory084 Whew! Off the hook. :-)

* if your transformer does not constrain its lifted effects would
* be `type TC[M[_]] = Unit =:= Unit`. A more common constraint
* might be `type TC[M[_]] = Monad[M]`.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Does this doc comment need to be updated to refer to Trivial?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Will fix.

@adelbertc
Copy link
Contributor

In light of the fixed build (#954) this need to be re-merged against the latest master. Sorry about that!

@codecov-io
Copy link

Current coverage is 90.98%

Merging #940 into master will increase coverage by +0.01% as of 037add7

@@            master    #940   diff @@
======================================
  Files          180     181     +1
  Stmts         2150    2153     +3
  Branches        42      42       
  Methods          0       0       
======================================
+ Hit           1956    1959     +3
  Partial          0       0       
  Missed         194     194       

Review entire Coverage Diff as of 037add7

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor

ceedubs commented Mar 28, 2016

This looks good to me so far. @djspiewak would you be willing to make that comment update to refer to Trivial and to add a test or two so that that the codecov isn't reporting a coverage drop?

@djspiewak
Copy link
Member Author

@ceedubs Maybe I'm misreading the coverage report, but as far as I can tell, there's literally nothing to add. All of the testable lines are already tested. The reason the coverage is dropping is I added untestable lines (literally: whitespace and type-level declarations), which for some reason, Codecov includes in the computation which generates the coverage percentage.

Again, I may simply be misreading it. If someone can point out to me what I missed, I will happily write tests. :-)

@adelbertc
Copy link
Contributor

It looks that way to me as well. 👍 from me!

@adelbertc
Copy link
Contributor

Hm given the fancy nature of these types, could you also add some tests in tests/src/test/scala/cats/tests/SyntaxTests.scala ? Not for code coverage, but just to check that they can be used nicely and make sure nobody breaks anything in the future.

@djspiewak
Copy link
Member Author

Well so much for your 👍! :-)

More seriously, the syntax is already tested by what previously existed. I could probably add one or two things that are now supported but which weren't before, but it wouldn't be too different from the tests we already have.

@adelbertc
Copy link
Contributor

I was halfway through amending my comment when I saw your new one :-)

Yeah I just realized there are pre-existing tests, I wrongly assumed this was a completely new addition. My 👍 stands!

@ceedubs
Copy link
Contributor

ceedubs commented Apr 3, 2016

👍 thanks, @djspiewak!

@ceedubs ceedubs merged commit 7cdd661 into typelevel:master Apr 3, 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