Uh oh!
There was an error while loading. Please reload this page.
Add Range#minmax specs - #777
Conversation
eregon
commented
Jul 4, 2020
To replace the monkey-patch, let's use a Maybe 2.7.2/2.7.3 could be used as upper bound, but first we need ruby/ruby#3285 (comment) then. We can always adjust it later. |
eregon
left a comment
There was a problem hiding this comment.
Looks good overall, thanks for the new specs!
| describe 'without enumerating the range' do | ||
| before(:each) do | ||
| # Prevent enumeration, but allow members to respond_to? :succ |
There was a problem hiding this comment.
Could you clarify like: to respond_to?(:succ) => false?
There was a problem hiding this comment.
It's actually the opposite. This ensures they respond_to?:succ, but that if it is ever actually called, the test fails.
The MRI implementation checks that it could iterate from the range beginning via .succ, even if it does not actually end up calling it, so we need to ensure the objects respond to it.
There was a problem hiding this comment.
Either way, it means I should clarify the comment.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @x = mock('x') | ||
| @y = mock('y') | ||
| @x.should_receive(:<=>).with(@y).any_number_of_times.and_return(-1) |
There was a problem hiding this comment.
A comment like # x < y would be helpful to read to understand the intent of those lines faster
This adds specs for Range#minmax, since the implementation provided by Enumerable is overidden as of 2.7, to try to skip enumerating the range.
sambostock
commented
Jul 7, 2020
After going through the code a little more, I realized there were some edge cases I didn't include. I'll update with those soon, but I got distracted by stumbling across another bug in one of the edge cases, this time in both |
eregon
left a comment
There was a problem hiding this comment.
The structure looks more readable now, thanks!
Regarding CI, Range#minmax on an inclusive range should raise RangeError on a beginless range seems a bug, can you file a ticket & use ruby_bug?
For Range#minmax on an exclusive range should raise RangeError on a beginless range it's just a different error message, so the spec should be adapted.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eregon
commented
Oct 24, 2020
@sambostock could you address my latest comments? Then it should be ready to merge. |
* Interestingly the previous commit worked fine on Ruby 2.7.2 on Linux but failed on macOS.
eregon
left a comment
There was a problem hiding this comment.
Thanks for the specs, I did the final touches to integrate it.
This adds specs documenting the behaviour of
Range#minmax, as updated in Ruby 2.7 (see #745). The specs specifically check that as of 2.7, simple ranges have theirminmaxcomputed in an optimized manner, without enumerating the range. They also check a few other cases, to ensure otherminmaxfunctionality works as expected.While writing these specs, I discovered a bug in the handling of non-numeric exclusive ranges in MRI, which I have submitted a fix for in ruby/ruby#3285. In the meantime, I have included a temporary monkey-patch, so the correct specs can be written.
Range#minmaxfix is releasedQuestions for Reviewers
Enumerable#minmax's specs should be duplicated?I initially had completely separate specs, but tried to reduce duplication by grouping them. I also experimented with shared examples, but I couldn't really get it to work and it produced specs that were unclear.