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

allow rand with zero value #3493

Merged
merged 1 commit into from Sep 10, 2017
Merged

allow rand with zero value #3493

merged 1 commit into from Sep 10, 2017

Conversation

kostya
Copy link
Contributor

@kostya kostya commented Nov 1, 2016

this is useful in such code:

def delay(interval : Float64)
  sleep(rand(interval))
end

# when you want just not to sleep
delay(0.0)

@RX14
Copy link
Contributor

@RX14 RX14 commented Nov 1, 2016

This is quite useful but i'm concerned that it could mask logic bugs in code that would otherwise raise.

@kostya
Copy link
Contributor Author

@kostya kostya commented Nov 1, 2016

why not?
also it already works for range: rand(0..0) => 0, rand(0.0..0.0) => 0.0, why not to work for zero itself?

@trans
Copy link

@trans trans commented Nov 8, 2016

This should definitely work. It provides an inherit on/off behavior, removing the need for extra conditionals, allowing code to be more succinct.

but i'm concerned that it could mask logic bugs in code that would otherwise raise.

That's what tests are for. Besides, if you are expecting behavior based on the generation of random numbers, it's usually pretty obvious when you just keep getting zeros.

@kostya
Copy link
Contributor Author

@kostya kostya commented Dec 15, 2016

👍

@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jun 28, 2017

@kostya This PR needs a rebase. Otherwise it looks good to me

@kostya
Copy link
Contributor Author

@kostya kostya commented Jun 29, 2017

rebased

@Sija
Copy link
Contributor

@Sija Sija commented Sep 10, 2017

RX14
RX14 approved these changes Sep 10, 2017
@asterite asterite merged commit 6202e37 into crystal-lang:master Sep 10, 2017
2 checks passed
@RX14 RX14 added this to the Next milestone Sep 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants