add spec for Array can be sliced with Enumerator::ArithmeticSequence #857

lxxxvi · 2021-10-06T07:44:42Z

Hello 👋

Today I'll try to cover this from #823 :

(Array) Can be sliced with Enumerator::ArithmeticSequence

Thank you for your feedback.

eregon

Thanks, looks great!

core/array/shared/slice.rb

lxxxvi · 2021-10-06T15:01:34Z

@eregon You made me overdo it 😂

I was playing around with almost any combinations of (start..end).step(step) and (start...end).step(step) (excluding the end value), where:

start : nil | 0 | 1 | 2 | [negative number]
end : nil | 0 | -1 | -2 | [positive number]
step : 1 | 2 | 10 | -1 | -2 | -10

Then I learned that Enumerator::ArithmeticSequence allows "inverted ranges", like (3..1).step(1) or (3..1).step(-1), which made it even more confusing.

I ended up with many constellations, some of which broke my brain, particularly where steps are negative.

For example (the last one only):

# @array = [0, 1, 2, 3, 4, 5]
@array.send(@method, eval("(2..).step(-1)")).should == [2, 1, 0]
@array.send(@method, eval("(2..).step(-2)")).should == [2, 0]
@array.send(@method, eval("(2..).step(-10)")).should == [0]                   # ??? I would have expected [2]

The same with entire blocks

it "has beginless range and negative steps" {}
it "has inverted closed range and negative steps" {}

Now, I have three questions for you:

Do expected values in the specs mentioned above make sense to you?
Can we clean out some specs?
Are you satisfied with the grouping of the specs, or do you want me to regroup them?

Regarding the clean out:

I think we can remove all assertions where the comment says:

# start and end with 0
# start with positive index, end with negative index
# start with negative index, end with positive index
# start with negative index, end with negative index

because I don't think they add value to the specs, what do you think?

eregon · 2021-10-07T16:00:26Z

@array.send(@method, eval("(2..).step(-10)")).should == [0]                   # ??? I would have expected [2]

does seem weird, could you report it to bugs.ruby-lang.org/?

eregon · 2021-10-07T16:01:24Z

😆 I was just asking to split in two, but more coverage is always welcome :)

eregon · 2021-10-07T16:06:25Z

https://rubyreferences.github.io/rubychanges/3.0.html#array-slicing-with-enumeratorarithmeticsequence might be useful

eregon

because I don't think they add value to the specs, what do you think?

I think they still increase coverage, and they run fast so I would keep them.

For all the ???, please report them and for now let's exclude those specs from this PR.
But please keep the current state on some other branch as they could be useful later on.
Maybe the behavior is actually intended for some of them although it's clearly very surprising to me too.

eregon · 2021-10-07T16:06:47Z

core/array/shared/slice.rb

+
+        @array.send(@method, eval("(2..).step(-1)")).should == [2, 1, 0]
+        @array.send(@method, eval("(2..).step(-2)")).should == [2, 0]
+        @array.send(@method, eval("(2..).step(-10)")).should == [0]                   # ???


I agree it's weird, please report it

My version produced:

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(2..).step(-10)]" [2]

(which seems reasonable, right?.. just the first one—at idx 2, then no more available items)

eregon · 2021-10-07T16:06:52Z

core/array/shared/slice.rb

+
+        @array.send(@method, eval("(-3..).step(-1)")).should == [3, 2, 1, 0]
+        @array.send(@method, eval("(-3..).step(-2)")).should == [3, 1]
+        @array.send(@method, eval("(-3..).step(-10)")).should == [0]                  # ???


I agree it's weird, please report it

Mine was:

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(-3..).step(-10)]" [3]

eregon · 2021-10-07T16:07:07Z

core/array/shared/slice.rb

+        @array.send(@method, eval("(-3..).step(-10)")).should == [0]                  # ???
+      end
+
+      # ????????


I agree it's weird, please report it

eregon · 2021-10-07T16:07:40Z

core/array/shared/slice.rb

+      # ????????
+      it "has inverted closed range and negative steps" do
+        # start and end with positive index
+        @array.send(@method, eval("(3..1).step(-1)")).should == [3, 2, 1]


This sounds reasonable

eregon · 2021-10-07T16:08:11Z

core/array/shared/slice.rb

+      it "has inverted closed range and negative steps" do
+        # start and end with positive index
+        @array.send(@method, eval("(3..1).step(-1)")).should == [3, 2, 1]
+        @array.send(@method, eval("(3...1).step(-1)")).should == [2, 1]


I agree it's weird, please report it. the 1 should be excluded, not the 3.

Mine was

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(3...1).step(-1)]" [3, 2]

eregon · 2021-10-07T16:11:21Z

@zverok @mrkn Could you take a look at the cases marked with ??? and see if they make sense to you or if they are bugs?

zverok

Left some comments. In general, I agree that most of what's marked with ??? is weird/should be reported as bugs :)

zverok · 2021-10-07T16:38:51Z

core/array/shared/slice.rb

+
+        @array.send(@method, eval("(2..).step(-1)")).should == [2, 1, 0]
+        @array.send(@method, eval("(2..).step(-2)")).should == [2, 0]
+        @array.send(@method, eval("(2..).step(-10)")).should == [0]                   # ???


My version produced:

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(2..).step(-10)]" [2]

(which seems reasonable, right?.. just the first one—at idx 2, then no more available items)

zverok · 2021-10-07T16:39:11Z

core/array/shared/slice.rb

+
+        @array.send(@method, eval("(-3..).step(-1)")).should == [3, 2, 1, 0]
+        @array.send(@method, eval("(-3..).step(-2)")).should == [3, 1]
+        @array.send(@method, eval("(-3..).step(-10)")).should == [0]                  # ???


Mine was:

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(-3..).step(-10)]" [3]

zverok · 2021-10-07T16:41:30Z

core/array/shared/slice.rb

+      # ????????
+      it "has beginless range and negative steps" do
+        # end with zero index
+        @array.send(@method, eval("(..0).step(-1)")).should == [5, 4, 3, 2, 1, 0]


For this, my version gave the same answer. And I agree it doesn't look reasonable (going from the -infinity to 0, in steps of -1, you should never arrive here).

zverok · 2021-10-07T16:53:43Z

core/array/shared/slice.rb

+      it "has inverted closed range and negative steps" do
+        # start and end with positive index
+        @array.send(@method, eval("(3..1).step(-1)")).should == [3, 2, 1]
+        @array.send(@method, eval("(3...1).step(-1)")).should == [2, 1]


Mine was

$ ./ruby --disable-gems -e "p [0, 1, 2, 3, 4, 5][(3...1).step(-1)]" [3, 2]

zverok · 2021-10-07T16:54:11Z

core/array/shared/slice.rb

+        @array.send(@method, eval("(...-2).step(-2)")).should == [5]
+
+        @array.send(@method, eval("(..-2).step(-10)")).should == [4]
+        @array.send(@method, eval("(...-2).step(-10)")).should == [4]


OK, all of those should give just [], it seems 🤷

zverok · 2021-10-07T18:29:20Z

core/array/shared/slice.rb

+        @array.send(@method, eval("(-2...-4).step(-2)")).should == [3]
+
+        @array.send(@method, eval("(-2..-4).step(-10)")).should == [2]
+        @array.send(@method, eval("(-2...-4).step(-10)")).should == [2]


In general, if I understand/remember the reasoning correctly, with a mix of negative and positive indexes the logic is "convert all indexes to positive (e.g. -1 to ary.length - 1), then iterate between them"

lxxxvi · 2021-10-08T15:40:35Z

@eregon

https://bugs.ruby-lang.org/issues/18247

I'll cleanup this branch, and copy the weird ones to a new branch on the weekend. 👍

lxxxvi · 2021-10-09T14:43:59Z

Hi @eregon

As you suggested, I moved the specs that were marked with ??? to another branch and removed them from this branch.

Letme know if you want any more changes, thank you!

eregon

Thank you for the great work on these specs!

lxxxvi · 2021-10-11T12:15:11Z

@eregon Thank you for your inputs and support!

eregon approved these changes Oct 6, 2021

View reviewed changes

core/array/shared/slice.rb Outdated Show resolved Hide resolved

eregon reviewed Oct 6, 2021

View reviewed changes

core/array/shared/slice.rb Outdated Show resolved Hide resolved

lxxxvi changed the title ~~add spec for Array can be sliced with Enumerator::ArithmeticSequence~~ WIP: add spec for Array can be sliced with Enumerator::ArithmeticSequence Oct 6, 2021

eregon reviewed Oct 7, 2021

View reviewed changes

zverok reviewed Oct 7, 2021

View reviewed changes

lxxxvi and others added 2 commits October 8, 2021 16:47

add spec for Array can be sliced with Enumerator::ArithmeticSequence

9d58f70

Add example with step 1

b5c2c20

lxxxvi force-pushed the array-can-be-sliced-with-arithmeticsequence branch from 8b7fe61 to d23057d Compare October 8, 2021 14:47

regroup Array#split specs using Enumerator::ArithmeticSequence

a3b33a8

lxxxvi force-pushed the array-can-be-sliced-with-arithmeticsequence branch from d23057d to a3b33a8 Compare October 9, 2021 14:33

lxxxvi changed the title ~~WIP: add spec for Array can be sliced with Enumerator::ArithmeticSequence~~ add spec for Array can be sliced with Enumerator::ArithmeticSequence Oct 9, 2021

eregon approved these changes Oct 11, 2021

View reviewed changes

eregon merged commit 17dbdfc into ruby:master Oct 11, 2021

lxxxvi deleted the array-can-be-sliced-with-arithmeticsequence branch January 12, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add spec for Array can be sliced with Enumerator::ArithmeticSequence #857

add spec for Array can be sliced with Enumerator::ArithmeticSequence #857

lxxxvi commented Oct 6, 2021

eregon left a comment

lxxxvi commented Oct 6, 2021

eregon commented Oct 7, 2021

eregon commented Oct 7, 2021

eregon commented Oct 7, 2021

eregon left a comment

eregon Oct 7, 2021

zverok Oct 7, 2021

eregon Oct 7, 2021

zverok Oct 7, 2021

eregon Oct 7, 2021

eregon Oct 7, 2021

eregon Oct 7, 2021

zverok Oct 7, 2021

eregon commented Oct 7, 2021

zverok left a comment

zverok Oct 7, 2021

zverok Oct 7, 2021

zverok Oct 7, 2021

zverok Oct 7, 2021

zverok Oct 7, 2021

zverok Oct 7, 2021

lxxxvi commented Oct 8, 2021

lxxxvi commented Oct 9, 2021

eregon left a comment

lxxxvi commented Oct 11, 2021

add spec for Array can be sliced with Enumerator::ArithmeticSequence #857

add spec for Array can be sliced with Enumerator::ArithmeticSequence #857

Conversation

lxxxvi commented Oct 6, 2021

eregon left a comment

Choose a reason for hiding this comment

lxxxvi commented Oct 6, 2021

eregon commented Oct 7, 2021

eregon commented Oct 7, 2021

eregon commented Oct 7, 2021

eregon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Oct 7, 2021

zverok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxxvi commented Oct 8, 2021

lxxxvi commented Oct 9, 2021

eregon left a comment

Choose a reason for hiding this comment

lxxxvi commented Oct 11, 2021