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.

Sign up for GitHub

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

Jump to bottom

Allow full Number range for chooseFloat #97

Merged
merged 2 commits into from
Dec 23, 2020
Merged

Allow full Number range for chooseFloat #97

merged 2 commits into from
Dec 23, 2020

Conversation

jacereda
Copy link
Contributor

No description provided.

@jacereda
Copy link
Contributor Author

Somewhat related, shouldn't there be a Bounded-like type-class where bottom/top are finite (Limited?), so that something like chooseFloat lower upper could be used?

@jacereda jacereda mentioned this pull request Jan 21, 2019
Merged
@hdgarrood
Copy link
Contributor

I’m not really a fan of a bounded-like type class where bottom/top are finite. What laws would it satisfy? Would it make sense for any types other than Number?

hdgarrood
hdgarrood approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Looks good to me; I just have one small comment.

test/Main.purs
@@ -84,6 +86,8 @@ main = do
quickCheck $ 4 >? 3
quickCheckFail $ 4 <=? 3

randomSample (MGen.chooseFloat ((-1.7976931348623157e+308)) (1.7976931348623157e+308)) >>= assert <<< all isFinite
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a log statement just before this line so that if this fails, it will be a little clearer as to why?

@JordanMartinez
Copy link
Contributor

This looks good to me. Just curious but why wasn't this PR merged previously?

@thomashoneyman thomashoneyman merged commit 48b33b4 into purescript:master Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdgarrood hdgarrood hdgarrood approved these changes

@JordanMartinez JordanMartinez JordanMartinez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jacereda @hdgarrood @JordanMartinez @thomashoneyman