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

added TFunctor #1815

Closed
wants to merge 7 commits into from
Closed

added TFunctor #1815

wants to merge 7 commits into from

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Aug 10, 2017

added instances for

  • WriterT
  • Kleisli
  • OptionT
  • EitherT
  • IdT
  • EitherK
  • Tuple2K
  • Free
  • FreeT
  • Func
  • Nested
  • OneAnd

Fixes #1812, #1713 and #1492. also sort of a continuation of #1620
Any instances I miss?
Question, do we need an instance for Free, not sure if there is a use case in the wild.

Update: Added instance for Free, EitherK, TupleK, Func, Nested and OneAnd

@iravid
Copy link
Contributor

iravid commented Aug 11, 2017 via email

@kailuowang
Copy link
Contributor Author

@iravid StateT's inner data has a nested F[_], i.e. F[S => F[(S, A)]], I don't know how to transform that to a G[S => G[(S, A)]]using just F~>G, any ideas?

added instances for WriterT, Kleisli, OptionT, EitherT and FreeT
@iravid
Copy link
Contributor

iravid commented Aug 11, 2017

Ah, you're right - I missed the part where StateT.transformF would need to impose more constraints on F. Oh well :-)

@codecov-io
Copy link

codecov-io commented Aug 11, 2017

Codecov Report

Merging #1815 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1815      +/-   ##
==========================================
+ Coverage   94.88%   94.94%   +0.05%     
==========================================
  Files         241      244       +3     
  Lines        4148     4193      +45     
  Branches      102      104       +2     
==========================================
+ Hits         3936     3981      +45     
  Misses        212      212
Impacted Files Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 92.85% <100%> (+0.66%) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 100% <100%> (ø) ⬆️
free/src/main/scala/cats/free/FreeT.scala 91.89% <100%> (+0.22%) ⬆️
core/src/main/scala/cats/data/EitherT.scala 98.3% <100%> (+0.02%) ⬆️
core/src/main/scala/cats/data/OneAnd.scala 98.41% <100%> (+0.05%) ⬆️
core/src/main/scala/cats/TFunctor.scala 100% <100%> (ø)
laws/src/main/scala/cats/laws/TFunctorLaws.scala 100% <100%> (ø)
...ain/scala/cats/laws/discipline/TFunctorTests.scala 100% <100%> (ø)
core/src/main/scala/cats/data/IdT.scala 97.29% <100%> (+0.15%) ⬆️
core/src/main/scala/cats/data/Tuple2K.scala 100% <100%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e7175...3f0f79d. Read the comment docs.

@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Aug 11, 2017
@@ -31,6 +31,11 @@ object Func extends FuncInstances {
}

private[data] abstract class FuncInstances extends FuncInstances0 {

implicit def catsDataTFunctorFoFunc[A]: TFunctor[Func[?[_], A, ?]] = new TFunctor[Func[?[_], A, ?]] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing "r" in "foFunc", similarly in "foNested".

@edmundnoble
Copy link
Contributor

I don't think we need to provide this type class to have a uniform API. We can just have a uniform API. Type classes are abstractions; unless there's a useful way to abstract over this, I think we should not include it. This is the standard for all other type classes in cats if I'm not mistaken.

@kailuowang
Copy link
Contributor Author

@edmundnoble
The law tests provided by the type class is more thorough than the existing test coverage (e.g. FreeT and Kleisili) - also plus many of the instances I added doesn't have a mapNT equivalent yet, not to mention the tests, so I would need to duplicate the same tests code for 12 instances.

I think the type class might be able to provide reusable code other than liftNT . I am thinking compose. I will try implement that and maybe try find more after vacation.

* as `OptionT`, `EitherT`
*
*/
@typeclass trait TFunctor[T[_[_], _]] {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need T[_[_], _] or would T[_[_]] do? I'm not sure what the second parameter is buying us. Maybe I'm missing it. Can't we just ignore the inner value type? Also, wouldn't we have more flexibility only acting on T[_[_]]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed here. If we had T[_[_]] I could come up with a lot more usecases. Maybe call it FunctorK then. We'd have to wrap the transformers in Forall though, which may mean our unified transform syntax fails to be found implicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason I chose T[_[_], _] here, is that given any Functor F[_], T[F, _] is also a Functor. So T[_[_], _] can be deemed as a functor between the F[_] Functor and the T[F, _] Functor. i.e. a endofunctor in the category of endofunctors, thus more like a higher kinded Functor.
If you use T[_[_]], then given any Functor F[_], T[F] isn't a Functor, it's a kind * type, not a type constructor. So T[_[_]] is a functor between F[_] Functor and kind * type T[F]s. That makes it less a high kinded Functor. However, you can work around with the parametricity in instance definitions, e.g. for OptionT,

def functorKInstance[A]: FunctorK[OptionT[?[_], A]]

With parametric A, we can have a FunctorK instance forall As. In some sense, we may choose to see this set of FunctorK instances as a single truely higher kinded functor.

Practical use wise, I can't think of any downside of using T[_[_]]. The only downside is its awkward encoding of the higher kinded Functor - it relies on instance definition. It may have more usecases, one that immediately jumps out is FunctionK. So maybe practically it's more useful.

Now, the fact that we aren't sure about this thing makes me think that maybe we shouldn't place this in cats-core. I already have FunctorK[T[_]] defined in mainecoon, I could add all these instances there. Or we can create a new cats-extra module for this type of less commonly used type class.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the example of FunctionK, my first thought is that that’s what’s defined incorrectly. We really want FunctionK[F[_], G[_], A] and Forall[FunctionK[F, G, ?]] for a natural transformation. Then you do have a FunctorK[FunctionK[F, ?[_], ?]]. But that might just be crazy talk.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if "higher-kinded X" is a really rigid term. In my mind, higher-kinded functor could equally well mean a functor from (endofunctors in Scal) to Scal, or a functor from endofunctors in Scal to other endofunctors in Scal. This is my main problem with the -K naming scheme; it gets even worse when type classes have multiple type parameters.

As well, the encoding proposed above does not reify that the A can vary with the instance remaining valid. The consumer of the instance needs a Forall[Lambda[A =>FunctorK[OptionT[?[_], A]]]] to have that fact in hand, and that makes it much more unwieldy, though a type alias could help.

To my mind if we're going to do a cats-extras project we'll want this there.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @kailuowang and @edmundnoble, I don't think it's all that useful in cats-core. I'd like to see conformity in Monad Transformers for defining a mapK method, but it doesn't need a type class in core IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if "higher-kinded X" is a really rigid term.

Absolutely. I don’t think it helps at all. But if Functor is really “endofunctor in the category of Scal”, I’d expect FunctorK to mean the same thing as K in other instances, which basically adds “endofunctors” to the description – i.e., “endofunctor in the category of endofunctors in Scal”.

I’d also be happy with Functor being renamed to Endofunctor and having EndofunctorK and something like LowerFunctor (for a functor from the category of endofunctors to the category of Skal).

Also agreed that none of this should be in cats-core. I would like to find some time to work with -Ykind-polymorphism and to define these things as specializations of kind-polymorphic endofunctors, etc.

This sort of thing has been started in a few different libraries already – khats (which is too homophonically-named to be useful), Griffins (which never really got off the ground and mostly lives inside a Matryoshka branch), and I think one other that I have forgotten 😕

@johnynek
Copy link
Contributor

+1 for FunctorK. Seems more consistent with cats naming styles.

@kailuowang
Copy link
Contributor Author

Closing this for now since 1.0 already released

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.

8 participants