-
Notifications
You must be signed in to change notification settings - Fork 43
Add genericArbitrary and genericCoarbitrary #63
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
Conversation
Looks great, although one possible downside is that this might make it difficult to remove |
Oh are there plans to remove these classes? |
It's been brought up in the past. Like Generic deriving picks a default implementation for each type, which I think is fine, but we need a class which we can use to implement it. So I'm not sure what the best solution is yet. One option could be to only use |
src/Test/QuickCheck/Arbitrary.purs
Outdated
coarbitrary NoArguments = id | ||
|
||
instance arbitrarySum :: (Arbitrary l, Arbitrary r) => Arbitrary (Sum l r) where | ||
arbitrary = arbitrary >>= if _ then Inl <$> arbitrary else Inr <$> arbitrary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One slight problem here is that this will not be evenly distributed if there are more than two constructors. The first would get chosen with 50% probability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, yes, interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we can fix this, we should merge this. Whether we decide to remove Arbitrary
or not later, we'll need to keep some class to support generic deriving anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an idea for solving this which I'm about to try now. Do we have the same problem with coarbitrary for product types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sum
never nests on the left, yeah? and would only ever contain Constructor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
@paf31 will we need to do anything similar for any of the coarbitrary instances? |
I don't think that's as big of a concern, as long as the generator gets perturbed somehow. But we could add it to the list of TODO items for later, perhaps. |
Since this adds a new dependency, I've marked it as breaking, and I think we should merge it in the next major release. |
This is only breaking because of the new dependency, which we've now decided is not breaking, so could you please rebase this one, and I'll release it? Thanks! |
Aren't we avoiding dependencies on either of the generics in the core libraries until there's a clear winner? |
My strong preference would be to deprecate |
Rebased and updated. |
@garyb Do you think there is a reason to keep I also have a PR open which defines and derives a class like the old Anyway, I suggest we merge this, since I think it could be very useful. What do you think? |
I'm maybe not the right person to ask, since I don't use either variety of generics for anything concrete; I've written a few instances for old-style and not touched new-style at all even. 😕 Given my indifference, I'm all for just choosing one at this point and deprecating the other, it's less confusing for everyone that way. 😄 |
Ok, in that case I'm going to merge this then, and we can try to make a plan for deprecating |
Thanks @LiamGoodacre! |
Someone in IRC was asking about these a little while ago, I thought it was a good idea to have them. Thoughts?