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

Revert "allow rand from zero value" #6686

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Sep 8, 2018

This reverts #3493.

That commit introduced a bad change: it silenced a valid error for an arbitrary case.
Saying "this is useful" while showing wrong code is not an argument; that code should have just used an inclusive range.

rand(n) is always equivalent to rand(0...n).

rand(0...0) even now says "Invalid range for rand: 0...0".

rand(0) asks for a number ≥ 0 and < 0. This makes no sense and should obviously be an error for the same reason.

This reverts commit 6202e37.

That commit introduced a bad change: it silenced a valid error for an
arbitrary case.

`rand(n)` is always equivalent to `rand(0...n)`.

`rand(0...0)` even now says "Invalid range for rand: 0...0".

`rand(0)` asks for a number >= 0 and < 0. This makes no sense and should
obviously be an error for the same reason.
@kostya
Copy link
Contributor

kostya commented Sep 8, 2018

but why rand(0..0) is ok?

@oprypin
Copy link
Member Author

oprypin commented Sep 8, 2018

It's "pick a number ≥ 0 and ≤ 0". That's 0.

oprypin added a commit to oprypin/crystal that referenced this pull request Sep 9, 2018
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

I agree, it's definitely an edge case and we should hide a possibility of error. In Ruby it's worse, it acts like rand (returning a float), which is super strange. Suggesting using an inclusive range is very good.

@ysbaddaden ysbaddaden merged commit 2eb4718 into crystal-lang:master Sep 10, 2018
@ysbaddaden ysbaddaden added this to the 0.27.0 milestone Sep 10, 2018
RX14 pushed a commit that referenced this pull request Sep 10, 2018
* Implement Random for BigInt

Fixes #5647

* spec typo fix

* Move to big_int.cr

* Use to_s in spec

* Remove special case of zero because it will be removed in #6686
ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
* Implement Random for BigInt

Fixes crystal-lang#5647

* spec typo fix

* Move to big_int.cr

* Use to_s in spec

* Remove special case of zero because it will be removed in crystal-lang#6686
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.

6 participants