-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move zip
and zip?
to Enumerable
and make it work with any number of Indexable
or Iterable
or Iterator
#7453
Conversation
017dab1
to
dec2fe3
Compare
…r of `Indexable` or `Iterable` or `Iterator`
dec2fe3
to
b7f7482
Compare
@r00ster91 Thanks! I found it easier to apply the suggestions locally and then force push everything. Let me know if it's good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! 👍
# a.zip(b, c) # => [{1, 4, 8}, {2, 5, 7}, {3, 6, 6}] | ||
# ``` | ||
def zip(*others : Indexable | Iterable | Iterator) | ||
pairs = Array(typeof(zip(*others) { |e| break e }.not_nil!)).new(size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that type resolution handle a other
collection whose elements are nilable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the tuple that becomes nilable because the compiler can't know whether the block will be called (or something like that). This doesn't affect individual elements.
I’m late to the review. But I wanted to say that i liked the flexibility this PR brings. And also the tricks of using typeof of the yielding methods to be able to type the resulting array. |
@bcardiff Oops, sorry I didn't wait for your review. I remembered you said it was approved but I forgot you wanted to review it. Yeah, typeof keeps rocking it :-) |
Because
Enumerable
only provides internal iteration (there's nonext
, unlikeIterator
) and I think because at the timezip
was written there was noIterator
at all,zip
used to only work againstIndexable
things.This PR changes that. First,
zip
is moved toEnumerable
because the only requirement the receiver has is to be able to traverse its elements. However, the arguments can now be any ofIndexable
,Iterable
orIterator
, and there can be any number of them.Some examples:
All of the above also work with the block version.
It's nice to see that the above snippet also works exactly the same as in Ruby :-)
It might be a bit strange that the receiver has to be
Enumerable
but the arguments have to be of different types, but the truth is that, at least in the std many/most of the type implementEnumerable
,Iterable
andIterator
at the same time.Is this useful? I'm pretty sure it has its usecases (zipping two or three sequences at the same time). It can also be good for "competition" problems. But ultimately, this is a more complete and powerful version of the existing
zip
, and the implementation isn't that complex.Bonus snippet: another way to do
each_with_index
:(but it's probably a bit slower than
each_with_index
)