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

Discard on failed pattern matches within a Generator #257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HuwCampbell
Copy link
Member

As discussed in #233.

@HuwCampbell HuwCampbell force-pushed the topic/discarding-fail branch from abd765b to 4121279 Compare March 20, 2019 11:12
@jacobstanley jacobstanley added the next breaking Merge on the next breaking change / major version label Mar 20, 2019
@fosskers
Copy link

fosskers commented May 3, 2019

A similar issue exists for PropertyT.

@moodmosaic
Copy link
Member

moodmosaic commented May 3, 2019

See this conversation regarding PropertyT https://github.com/hedgehogqa/haskell-hedgehog/pull/233/files#r234163334

@jacobstanley
Copy link
Member

jacobstanley commented May 13, 2019

I've spent some time thinking about this and the problem I have with it is that ideally one should aim to write tests / generators that don't ever discard.

Discarding slows things down and I would consider its use a bit of an anti-pattern. I think it should only really be used as a last resort. You're better of using the just / filter functions which locally retry the generation so that your "discard" is limited in scope, rather than throwing away the entire test because some deeply nested generator has failed.

just :: MonadGen m => m (Maybe a) -> m a
filter :: MonadGen m => (a -> Bool) -> m a -> m a

So I'm just a bit unsure about making it syntactically convenient to discard, as often there are more appropriate options. I actually wonder if it would be better to remove the MonadFail instance for Gen and have it be a compile error. This would mean you can't use it for the "this should never happen" scenario though, which you could argue isn't going to cause any huge issues (like discarding does).

@moodmosaic
Copy link
Member

I actually wonder if it would be better to remove the MonadFail instance for Gen and have it be a compile error.

This aligns pretty well with @sol's comment on the same topic (QC): nick8325/quickcheck#228 (comment)

If we decide to get rid of MonadFail perhaps we better do now, before next major release is out.

@jacobstanley
Copy link
Member

If we decide to get rid of MonadFail perhaps we better do now, before next major release is out.

Too late 😅

I think this topic has too many trade-offs to rush the decision, so I'm not that concerned. What we ended up releasing is the same as what it has always been.

@jacobstanley jacobstanley force-pushed the master branch 2 times, most recently from 4139585 to c228279 Compare May 22, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next breaking Merge on the next breaking change / major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants