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

Functor composition. #165

Merged
merged 5 commits into from
Feb 12, 2015
Merged

Functor composition. #165

merged 5 commits into from
Feb 12, 2015

Conversation

coltfred
Copy link
Contributor

I've ported the code from @mpilquist's Structure with some slight modifications. I didn't write additional tests and decided it'd be more valuable to open the PR rather than waiting to hear back in chat about the correct way to add this kind of thing to the test suite.

I'm not sure given the correct types that it's possible to write the compositions incorrectly, but it might be a good idea to express that in some laws.

@coltfred
Copy link
Contributor Author

This is to fix #148.

new CompositeApply[F,G] {
implicit def F: Apply[F] = self
implicit def G: Apply[G] = GG
def compose[G[_]: Apply]: Apply[Lambda[X => F[G[X]]]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

In #27 it sounds like the agreed upon convention (that we still need to add to the contributor guide) is to use the implicit GG: Apply[G] approach instead of using G[_]: Apply and then calling Apply[G]. Could you please change this back to the previous form? The change from type lambda to Lambda is good - keep that change.

Could you also do the same change in the other places in this diff where <Typeclass>.apply is being called to fetch the implicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing! Thanks for looking at it!

@ceedubs
Copy link
Contributor

ceedubs commented Feb 10, 2015

Thanks!

Travis is complaining about some trailing whitespace in Functor.scala. Other than this and my one inline comment I think this is good, but I haven't yet looked at it in detail.


/**
* Compose this functor F with a Contravariant Functor G to produce a new Contravariant Functor
* on G[F[_]].
Copy link
Contributor

Choose a reason for hiding this comment

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

F[G[_]] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Fixed.

@coltfred
Copy link
Contributor Author

Travis was happy, but then I removed my change to InvariantLaws because it wasn't suppose to be there. Everything should be good and all questions above have been addressed.

@non
Copy link
Contributor

non commented Feb 10, 2015

Seems good. 👍

@stew
Copy link
Contributor

stew commented Feb 12, 2015

👍

@ceedubs
Copy link
Contributor

ceedubs commented Feb 12, 2015

@coltfred sorry, but could you please sync this with master? After that I think it will be good to merge.

Conflicts:
	core/src/main/scala/cats/Apply.scala
@coltfred
Copy link
Contributor Author

@ceedubs I merged master into this PR.

@ceedubs
Copy link
Contributor

ceedubs commented Feb 12, 2015

@coltfred thanks! 👍

ceedubs added a commit that referenced this pull request Feb 12, 2015
@ceedubs ceedubs merged commit b96ea94 into typelevel:master Feb 12, 2015
@coltfred coltfred deleted the FunctorVariance branch February 12, 2015 16:08
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