-
Notifications
You must be signed in to change notification settings - Fork 107
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
Change Applicative GenT to use zipping #272
Change Applicative GenT to use zipping #272
Conversation
52eadc8
to
b08ff0c
Compare
b08ff0c
to
311ad28
Compare
311ad28
to
fcfc320
Compare
fcfc320
to
53b87ba
Compare
hmm can't use CPP |
It's wild that the scala commits are showing up here, pretty cool |
I decided requiring 7.10 has been a real pain recently though, these last few PRs always seem to have some issue. It is pretty old now! Version 7.10.3 (released 8th December 2015) |
I guess we should consider dropping GHC 7.* when GHC 9.* comes out, which might not be so far away given the increased GHC release cycles. |
This changes the
Applicative
instance forGenT
so that it zips the tree rather than binding it. This means that a generator like(,) <$> genA <*> genB
will be able to alternate between shrinkinggenA
andgenB
rather than doing one first and then the other.An example shrink tree using a zipping approach vs just using
(<*>) = ap
:I had it like this originally and changed it because it broke the law
<*> = ap
, but I'm not sure this is that important as theMonad
instance forGenT
is already unlawful.Haxl also uses an unlawful
Applicative
instance for basically the same reason. This was the motivation forApplicativeDo
which you would be able to use with Hedgehog after this patch to achieve more optimal shrinking.A nice article explaining another option and the tradeoffs: https://elvishjerricco.github.io/2016/09/17/abstracting-async-concurrently.html
I also renamed
Tree
toTreeT
, so I could haveTree = TreeT Identity
as I was working withTreeT Identity
a lot during testing and it got a bit tedious.