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

Add note about enumBounded #202

Merged
merged 1 commit into from
Mar 10, 2019
Merged

Add note about enumBounded #202

merged 1 commit into from
Mar 10, 2019

Conversation

thumphries
Copy link
Member

'enumBounded' is partial in some cases. Usually you actually want something like Gen.linear minBound maxBound or similar. The error message is pretty inscrutable when you mess it up (no source locations), so I think a cautionary message is worthwhile.

Bikeshed on wording welcome

'enumBounded' is partial in many cases.
@jacobstanley
Copy link
Member

Maybe we can do a check and give an error which is more sensible?

Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

I think we need both―a note in the comments, so it shows up in the docs, and also a check with an error message (in the error message we can put that same stuff as in the notes).

@moodmosaic
Copy link
Member

Should be OK to merge this as-is and have the check on a different PR, if that's easier.

@moodmosaic
Copy link
Member

Bikeshed on wording welcome

Hmm, perhaps we should mention what the docs say for fromEnum as well?

Convert to an Int. It is implementation-dependent what fromEnum returns when applied to a value that is too large to fit in an Int.

@jacobstanley
Copy link
Member

I'm not sure the fromEnum docs add that much? it kind of implies that it might actually work when it probably won't, it is "more correct" but I think just exploding is probably what is going to happen.

@moodmosaic
Copy link
Member

moodmosaic commented Jun 13, 2018

Because it's fromEnum that isn't total at the end:

diff --git a/hedgehog/test/Test/Hedgehog/Text.hs b/hedgehog/test/Test/Hedgehog/Text.hs
index 0a11e91..a320100 100644
--- a/hedgehog/test/Test/Hedgehog/Text.hs
+++ b/hedgehog/test/Test/Hedgehog/Text.hs
@@ -29,7 +29,7 @@ genOdd =

 genSeed :: Gen Seed
 genSeed =
-  Seed <$> Gen.word64 Range.constantBounded <*> fmap fromIntegral genOdd
+  Seed <$> Gen.enumBounded <*> fmap fromIntegral genOdd

 genPrecedence :: Gen Int
 genPrecedence =
[thread killed]
test.exe:
  Enum.fromEnum{Word64}:
    value (18446744073709551615) is outside of Int's bounds
      (-9223372036854775808,9223372036854775807)

But yeah you're probably right that we might not need to get into such detail.

@thumphries
Copy link
Member Author

Will mess around and try to produce a better error.

Happy to kill the documentation if y'all prefer. I hadn't quite joined the dots that this had to be going via fromEnum, so I definitely overuse this function where I probably shouldn't.

@moodmosaic
Copy link
Member

Yeah, I think adding a check in order to provide a reasonable error is better. But the notes you've added I think will show up nicely in the docs 😃

image

vs

image

It's up to you, though. 🎸

@jacobstanley
Copy link
Member

I think the error would be good, but no reason to hold up this comment.

@jacobstanley jacobstanley merged commit d17958b into master Mar 10, 2019
@jacobstanley jacobstanley deleted the enumBounded-docs branch March 10, 2019 03:10
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