Skip to content

Add genericArbitrary and genericCoarbitrary#63

Merged
paf31 merged 1 commit into
purescript:masterfrom
LiamGoodacre:feature/generic-gen
Jun 3, 2017
Merged

Add genericArbitrary and genericCoarbitrary#63
paf31 merged 1 commit into
purescript:masterfrom
LiamGoodacre:feature/generic-gen

Conversation

@LiamGoodacre

Copy link
Copy Markdown
Member

Someone in IRC was asking about these a little while ago, I thought it was a good idea to have them. Thoughts?

@paf31

paf31 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

Looks great, although one possible downside is that this might make it difficult to remove (Co)Arbitrary later.

@LiamGoodacre

Copy link
Copy Markdown
Member Author

Oh are there plans to remove these classes?

@paf31

paf31 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

It's been brought up in the past. Like IsForeign, the issue is that there is rarely a single canonical implementation for a given type.

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 (Co)Arbitrary for deriving purposes, and I'd be fine with that, but it would need to be documented.

@paf31 paf31 mentioned this pull request Jan 7, 2017
Comment thread 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

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ooh, yes, interesting.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member Author

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?

@paf31 paf31 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks!

@LiamGoodacre

Copy link
Copy Markdown
Member Author

@paf31 will we need to do anything similar for any of the coarbitrary instances?

@paf31

paf31 commented Jan 29, 2017

Copy link
Copy Markdown
Contributor

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.

@paf31 paf31 changed the title Add genericArbitrary and genericCoarbitrary [BREAKING] Add genericArbitrary and genericCoarbitrary Jan 29, 2017
@paf31

paf31 commented Jan 29, 2017

Copy link
Copy Markdown
Contributor

Since this adds a new dependency, I've marked it as breaking, and I think we should merge it in the next major release.

@paf31 paf31 mentioned this pull request Apr 4, 2017
@paf31

paf31 commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

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!

@paf31 paf31 changed the title [BREAKING] Add genericArbitrary and genericCoarbitrary Add genericArbitrary and genericCoarbitrary Jun 3, 2017
@garyb

garyb commented Jun 3, 2017

Copy link
Copy Markdown
Member

Aren't we avoiding dependencies on either of the generics in the core libraries until there's a clear winner?

@paf31

paf31 commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

My strong preference would be to deprecate generics in favor of generics-rep at this point.

@LiamGoodacre

Copy link
Copy Markdown
Member Author

Rebased and updated.

@paf31

paf31 commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

@garyb Do you think there is a reason to keep generics around? I'm fairly sure the only benefit generics has is that you can reify a Generic instance more easily from a data structure without some sort of constraint kind trickery. However, I've never seen anyone use that technique, and it's very cumbersome to do so. I closed my PR which implemented it.

I also have a PR open which defines and derives a class like the old Generic using the new Generic. It might be possible to add the reifySignature function there instead, which would answer this nicely.

Anyway, I suggest we merge this, since I think it could be very useful. What do you think?

@garyb

garyb commented Jun 3, 2017

Copy link
Copy Markdown
Member

Do you think there is a reason to keep generics around?

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. 😄

@paf31

paf31 commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

Ok, in that case I'm going to merge this then, and we can try to make a plan for deprecating generics or merging generics-rep in at some point.

@paf31 paf31 merged commit 56eb222 into purescript:master Jun 3, 2017
@paf31

paf31 commented Jun 3, 2017

Copy link
Copy Markdown
Contributor

Thanks @LiamGoodacre!

@LiamGoodacre LiamGoodacre deleted the feature/generic-gen branch June 3, 2017 23:36
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.

3 participants