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

Support Time ranges in rand #5781

Merged
merged 2 commits into from Jul 5, 2019

Conversation

@davishmcclurg
Copy link
Contributor

commented Jul 2, 2019

JRuby is currently converting Time objects to floats when generating a
random value from a range:

$ RBENV_VERSION=jruby-9.2.7.0 ruby -e 'puts rand(Time.now..Time.now)'
1562034267.423605

MRI returns a Time object:

$ RBENV_VERSION=2.6.3 ruby -e 'puts rand(Time.now..Time.now)'
2019-07-01 19:27:25 -0700

I believe the RubyNumeric check I added is the same as what MRI uses
in rb_check_to_float: https://github.com/ruby/ruby/blob/0b858425e1e4f2de40dc0d8e5dd105a2fd93e478/object.c#L3769

Support Time ranges in rand
JRuby is currently converting Time objects to floats when generating a
random value from a range:

```
$ RBENV_VERSION=jruby-9.2.7.0 ruby -e 'puts rand(Time.now..Time.now)'
1562034267.423605
```

MRI returns a Time object:

```
$ RBENV_VERSION=2.6.3 ruby -e 'puts rand(Time.now..Time.now)'
2019-07-01 19:27:25 -0700
```

I believe the `RubyNumeric` check I added is the same as what MRI uses
in `rb_check_to_float`: https://github.com/ruby/ruby/blob/0b858425e1e4f2de40dc0d8e5dd105a2fd93e478/object.c#L3769
@kares

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

specs look good but if you look at the build the proposed fix introduced regressions: https://api.travis-ci.org/v3/job/553080882/log.txt ... the 'proper' fix likely won't involve a change a TypeConverter

Return float from `CustomRangeFloat#-`
This looks to be the behavior that MRI expects for float-like values in
ranges.

MRI calls `range.end - range.begin` when generating a random value from
a range: https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/random.c#L1051

If the return value responds to `to_int` it is treated as an integer, so
`CustomRangeInteger` works fine:
https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/object.c#L3243

Float-like objects that respond to `to_f` are treated differently and must be numeric:
https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/object.c#L3769
@davishmcclurg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@kares I think the fix is correct (TypeConverter.checkFloatType is only used in randomRand as far as I can tell), but the CustomRangeFloat check I added wasn't right. I pushed up another commit to fix it. Let me know if I should squash them.

@kares kares added this to the JRuby 9.2.8.0 milestone Jul 5, 2019

@kares kares merged commit a245812 into jruby:master Jul 5, 2019

1 of 2 checks passed

jruby.jruby Build #20190703.1 failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

eregon added a commit to ruby/spec that referenced this pull request Jul 27, 2019

Support Time ranges in rand (jruby/jruby#5781)
* Support Time ranges in rand

JRuby is currently converting Time objects to floats when generating a
random value from a range:

```
$ RBENV_VERSION=jruby-9.2.7.0 ruby -e 'puts rand(Time.now..Time.now)'
1562034267.423605
```

MRI returns a Time object:

```
$ RBENV_VERSION=2.6.3 ruby -e 'puts rand(Time.now..Time.now)'
2019-07-01 19:27:25 -0700
```

I believe the `RubyNumeric` check I added is the same as what MRI uses
in `rb_check_to_float`: https://github.com/ruby/ruby/blob/0b858425e1e4f2de40dc0d8e5dd105a2fd93e478/object.c#L3769

* Return float from `CustomRangeFloat#-`

This looks to be the behavior that MRI expects for float-like values in
ranges.

MRI calls `range.end - range.begin` when generating a random value from
a range: https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/random.c#L1051

If the return value responds to `to_int` it is treated as an integer, so
`CustomRangeInteger` works fine:
https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/object.c#L3243

Float-like objects that respond to `to_f` are treated differently and must be numeric:
https://github.com/ruby/ruby/blob/5d9e91afe08c470485333f6c6e034d05ea3ee908/object.c#L3769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.