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

Define laws for Traversable #442

Closed
wants to merge 17 commits into from
Closed

Conversation

zalbia
Copy link
Contributor

@zalbia zalbia commented Nov 25, 2020

closes #373

@zalbia zalbia changed the title define identity law for Traversable Define laws for Traversable Nov 25, 2020
/**
* Traversing by `Id` is equivalent to mapping.
*/
val identityLaw: LawsF.Covariant[DeriveEqualTraversable, Equal] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can implement your own law until we add one to ZIO Test.

@@ -269,11 +270,25 @@ trait Traversable[F[+_]] extends Covariant[F] {

object Traversable extends LawfulF.Covariant[DeriveEqualTraversable, Equal] {

// need to implement natural transformations for this property?
// val naturalityLaw: LawsF.Covariant[DeriveEqualTraversable, Equal] = ???
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can restate this as a fusion law that if we have F[G[H[A]]] where F is traversable and G and H have IdentityBoth and Covariant then we can individually flip twice or we can create a composed instance and flip the whole thing once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the big tip!

@zalbia
Copy link
Contributor Author

zalbia commented Nov 28, 2020

@adamgfraser can you check if I'm going in the right direction so far? AFAICT, there's still lots of work to be done, like making it so the Traversable laws run with Covariant's, as well as adding some more instances to check maybe?

- make newtypes for `Nested` functors and functor `Product`s
- make instances needed for everything to work
- add type ascriptions to make compiler happy in Traversable.scala
@zalbia zalbia marked this pull request as ready for review November 30, 2020 16:49
*
* If F[_] and G[_] are both IdentityBoth, then Nested[F, G, *] is also an IdentityBoth.
*
* If F[_] and G[_] are both Traversable, then Nested[F, G, *] is also a Traversable.

Choose a reason for hiding this comment

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

I think it would be nice to add tests for that at CovariantSpec, IdentityBothSpec, TraversableSpec

can copy it from my branch:
Badmoonz@6df1301#diff-82e9bed52abfcc1cabd8fb86c811dcab611fbd08d2ff81124a90af2fb0a78834R21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried to add the ones for CovariantSpec & IdentityBothSpec, and they led to the same build errors for 2.11 in #449. Any idea on how to make them work?

Copy link

@Badmoonz Badmoonz Dec 5, 2020

Choose a reason for hiding this comment

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

After some research , I've found solution : make alias type for nested

      {
        type T[+A] = Nested[Vector, Option]
        testM("Nested[vector,option]")(checkAllLaws(Traversable)(GenFs.nested(GenF.vector, GenF.option) : GenF[T, Random with Sized] , Gen.anyInt))
       }

It looks like scala 2.11 compiler cannot derive type CapF[_[_[_]]] from first argument GenFs.nested(GenF.vector, GenF.option) which has type GenF[(lambda[+A] = Nested[Vector, Option, A)#lambda, Random with Sized].
I supposed that's because it has lambda type param, but CovariantSpec has tuple, either gens that also return GenF with lambda type param, and it works well

@jdegoes
Copy link
Member

jdegoes commented Dec 15, 2020

@adamgfraser @zalbia @sideeffffect This one ready to merge (aside from merge conflicts)???

# Conflicts:
#	core/shared/src/test/scala/zio/prelude/TraversableSpec.scala
@sideeffffect
Copy link
Member

@jdegoes it may turn out we may need just one law for Traversable #373 (comment)

@adamgfraser
Copy link
Contributor

@Badmoonz @zalbia You have both done good work in this and I believe some of the work by @zalbia on this has been incorporated into the PR from @Badmoonz. How would you like to move forward to get this over the finish line?

@zalbia
Copy link
Contributor Author

zalbia commented Dec 17, 2020

We should merge #449 once the PR passes checks.

@adamgfraser
Copy link
Contributor

@zalbia 👍

@Badmoonz
Copy link

ok, i will update und fix #449 today

@zalbia zalbia closed this Jan 12, 2021
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.

Define laws for Traversable
5 participants