Skip to content

Conversation

@johnynek
Copy link
Contributor

@johnynek johnynek commented Oct 3, 2017

These are two methods I often want with Gen.

I think the tailRecM is correct, but I don't know the internal implementation that well so happy to get any feedback on how to improve it.

cc @non

@johnynek
Copy link
Contributor Author

johnynek commented Oct 3, 2017

okay, I am much more confident that this is correct now.


/** Build a pair from two generators
*/
def product[A, B](ga: Gen[A], gb: Gen[B]): Gen[(A, B)] =
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation is a bit different, but there is a zip method. Is this intended to be available in addition to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly, I didn't know about it. I think this implementation is more efficient but I am happy to remove it (or change the zip 2 implementation to use this approach).

My main interest in the tailRecM.

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnynek just to provide some context for that last comment: I'm not a Scalacheck maintainer; I just happened to see this change and I was pretty sure that a method similar to product existed, so I thought that I would point it out. I have no opinions on how to proceed regarding product, but I certainly could see benefit in tailRecM existing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove product here and devote the energy to porting that strategy over to the generated zip implementations. I agree with @johnynek that this approach is probably more efficient, so let's definitely do that.

seeds.forall { seed =>
g1.pureApply(params, seed) == finalG2.pureApply(params, seed)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great property!

@non
Copy link
Contributor

non commented Oct 4, 2017

Once we remove product I'm +1 on merging this.

@non
Copy link
Contributor

non commented Oct 4, 2017

👍

@non non merged commit 4cf7f8f into typelevel:master Oct 4, 2017
@johnynek johnynek deleted the oscar/tailrecm branch October 4, 2017 22:45
@johnynek johnynek changed the title Add Gen.product and Gen.tailRecM Add Gen.tailRecM Oct 4, 2017
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.

4 participants