Eliminate FunctorT and TraverseT type classes. #51

sellout · 2016-12-11T23:54:50Z

As well as other changes needed in the wake of the directly-recursive
overhaul – like fixing EqualT/ShowT implicits, etc.

As well as other changes needed in the wake of the directly-recursive overhaul.

sellout · 2016-12-11T23:56:36Z

@paulp This is “in progress” because there may still be a couple changes as I finish updating Quasar to use it, but it should be pretty solid.

(And apologies to @tpolecat for breaking EqualT, etc. for a bit, there)

paulp · 2016-12-12T01:49:28Z

Looks like a great simplification so far.

sellout · 2016-12-12T03:53:16Z

Yeah, so some comments in that vein …

RecursiveT/Co…T/Bi…T still exist for a few reasons – they make it easy to slowly transition Quasar, they are helpful in faking mutual recursion until that is in, and BirecursiveT works while Birecursive doesn’t, so that’s also nice;
having Birecursive working (Add Birecursive type class #44) would help clean up a bunch of stuff; and
@milessabin’s work on multiple implicit lists will clean up soooo much stuff.

sellout · 2016-12-12T20:18:04Z

core/shared/src/main/scala/matryoshka/Recursive.scala

@@ -301,6 +256,185 @@ trait Recursive[T] extends Based[T] {
    (implicit R: Corecursive.Aux[R, Base], BF: Functor[Base])
      : R =
    cata[R](t)(R.embed(_))
+
+  def mapR[U, G[_]: Functor]


This and below is pulled from FunctorT and TraverseT – it seemed prudent to not get rid of the operations yet (#45 is tracking this), since we use them a bunch in Quasar and I’m just not sure what the right approach is.

There are also some operations moved from earlier in the type class – into the group of things that should be in Birecursive once that is actually usable.

sellout · 2016-12-12T20:20:58Z

core/shared/src/main/scala/matryoshka/implicits/IdOps.scala

@@ -28,9 +28,9 @@ sealed class IdOps[A](self: A) {
    matryoshka.hyloM(self)(f, g)

  object ghylo {
-    def apply[W[_], N[_]] = new Aux[W, N]
+    def apply[W[_], N[_]] = new PartiallyApplied[W, N]


Renamed these “helper” classes to match the Cats approach, which is named this way to avoid confusion with the other Aux pattern. Figured since I was adding new ones, I should make sure the code is consistent.

This is something that multiple implicit lists would make disappear.

sellout · 2016-12-12T20:23:26Z

core/shared/src/main/scala/matryoshka/package.scala

-    * @group algebras 
-    */
-  type AlgebraicTransform[T[_[_]], F[_], G[_]]               = F[T[G]] => G[T[G]]    // GAlgebraicTransformM[T, Id, Id, F, G]
+  type AlgebraicGTransform[W[_], T, F[_], G[_]]        = F[W[T]] => G[T]


These new aliases just use the new proper type approach. It means the differ a bit (e.g., the types of AlgebraicTransform and CoalgebraicTransform are now the same, so they’re just Transform).

sellout · 2016-12-12T20:26:34Z

tests/src/test/scala/matryoshka/instances/fixedpoint/partial.scala

@@ -69,7 +69,7 @@ class PartialSpec extends Specification with ScalazMatchers with ScalaCheck with
    "return after multiple runs" >> prop { (a: Conat, b: Conat) =>
      val (ai, bi) = (a.cata(height), b.cata(height))
      bi > 0 ==> {
-        val first = (a + b).transAna(Partial.delay(27)).runFor(ai)
+        val first = (a + b).transAna[Partial[Int]](Partial.delay(27)).runFor(ai)


These operations (like the non-trans operations with the same names) now require a type annotation, because they only know that the result type has a particular functor, but not what proper type that functor has been extracted from.

sellout · 2016-12-12T20:27:41Z

tests/src/test/scala/matryoshka/patterns/diff.scala

-            Different[Mu, Example, Mu[Diff[Mu, Example, ?]]](Empty[Mu[Example]]().embed, NonRec[Mu[Example]]("x", 3).embed).embedT,
-            Removed[Mu, Example, Mu[Diff[Mu, Example, ?]]](Empty[Mu[Example]]().embed).embedT),
+            Different[Mu, Example, Mu[Diff[Mu, Example, ?]]](Empty[Mu[Example]]().embed, NonRec[Mu[Example]]("x", 3).embed).embed,
+            Removed[Mu, Example, Mu[Diff[Mu, Example, ?]]](Empty[Mu[Example]]().embed).embed),


With the fixed implicits, we can use the *ecursive ops rather than the *ecursiveT ops, so now we no longer use the T ops, and in fact, no longer have an implicit conversion that makes them available.

codecov-io · 2016-12-12T20:36:02Z

Current coverage is 72.67% (diff: 71.18%)

Merging #51 into master will decrease coverage by 0.82%

@@             master        #51   diff @@
==========================================
  Files            38         37     -1   
  Lines           649        666    +17   
  Methods         634        649    +15   
  Messages          0          0          
  Branches         15         17     +2   
==========================================
+ Hits            477        484     +7   
- Misses          172        182    +10   
  Partials          0          0

Powered by Codecov. Last update 150b572...8cbe2e5

sellout · 2016-12-12T20:37:24Z

Ok, @paulp now it‘s yours.

Once this gets in, I’ll submit the PR that updates Quasar to use this version.

paulp · 2016-12-12T20:50:54Z

@sellout shouldn't CorecursiveT also lose its @typeclass annotation?

paulp · 2016-12-12T21:19:47Z

@sellout what about 06e82db, is that okay?

sellout · 2016-12-12T21:20:44Z

Ah yes, that is great :)

paulp · 2016-12-12T21:42:15Z

I think that's everything concrete I have words for at the moment. Do you want me to make the CorecursiveT change?

sellout · 2016-12-12T21:51:38Z

Sure. I like this new thing in the space between code review and pairing 😄

paulp · 2016-12-12T21:57:09Z

@sellout I think it's called code repairing

paulp · 2016-12-12T21:58:55Z

I'll merge it after the tests pass.

Eliminate FunctorT and TraverseT type classes.

8cbe2e5

As well as other changes needed in the wake of the directly-recursive overhaul.

sellout added the status: In progress label Dec 11, 2016

sellout assigned paulp Dec 11, 2016

sellout added status: In review and removed status: In progress labels Dec 11, 2016

sellout added status: In progress and removed status: In review labels Dec 12, 2016

sellout commented Dec 12, 2016

View reviewed changes

sellout added status: In review and removed status: In progress labels Dec 12, 2016

Consolidate some CorecursiveT/RecursiveTs.

06e82db

Drop @typeclass annotation from CorecursiveT.

a2fb5f0

paulp merged commit a2fb5f0 into precog:master Dec 12, 2016

sellout removed the status: In review label Dec 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate FunctorT and TraverseT type classes. #51

Eliminate FunctorT and TraverseT type classes. #51

sellout commented Dec 11, 2016

sellout commented Dec 11, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

sellout Dec 12, 2016

sellout Dec 12, 2016

sellout Dec 12, 2016

sellout Dec 12, 2016

sellout Dec 12, 2016

codecov-io commented Dec 12, 2016

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

paulp commented Dec 12, 2016

Eliminate FunctorT and TraverseT type classes. #51

Eliminate FunctorT and TraverseT type classes. #51

Conversation

sellout commented Dec 11, 2016

sellout commented Dec 11, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

sellout Dec 12, 2016

Choose a reason for hiding this comment

sellout Dec 12, 2016

Choose a reason for hiding this comment

sellout Dec 12, 2016

Choose a reason for hiding this comment

sellout Dec 12, 2016

Choose a reason for hiding this comment

sellout Dec 12, 2016

Choose a reason for hiding this comment

codecov-io commented Dec 12, 2016

Current coverage is 72.67% (diff: 71.18%)

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

sellout commented Dec 12, 2016

paulp commented Dec 12, 2016

paulp commented Dec 12, 2016