-
-
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
Documenting Indexable#zip
and Indexable#zip?
methods.
#5734
Documenting Indexable#zip
and Indexable#zip?
methods.
#5734
Conversation
src/indexable.cr
Outdated
@@ -412,24 +412,80 @@ module Indexable(T) | |||
indexes.map { |index| self[index] } | |||
end | |||
|
|||
# Calls the given block once for each index in `self`, passing `self` |
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.
Calls the given block for each index in
self
, passing in the element at that index inself
and the element at the same index inother
.
?
Also make sure to note that this method raises.
src/indexable.cr
Outdated
# a = [1, 2, 3] | ||
# b = ["a", "b", "c"] | ||
# | ||
# a.zip(b) { |x,y| puts "[ #{x} -- #{y} ]" } |
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.
just "#{x} -- #{y}"
? Not sure why we need the []
.
src/indexable.cr
Outdated
def zip(other : Indexable) | ||
each_with_index do |elem, i| | ||
yield elem, other[i] | ||
end | ||
end | ||
|
||
# Returns an `Array` of Tuples populated with the *N* index | ||
# element from `self` and *N* index element from `other`. |
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.
Returns an
Array
of tuples populated with the ith indexed element fromself
followed by the ith indexed element fromother
, for all i0..self.size - 1
Maybe that needs more tweaking.
src/indexable.cr
Outdated
# Returns an `Array` of Tuples populated with the *N* index | ||
# element from `self` and *N* index element from `other`. | ||
# | ||
# Raises IndexError if `other` has no index corresponding to `self`. |
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.
I'd mention that this happens when other
is shorter than self
.
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.
Makes sense. It makes the sentence more clear.
src/indexable.cr
Outdated
def zip(other : Indexable(U)) forall U | ||
pairs = Array({T, U}).new(size) | ||
zip(other) { |x, y| pairs << {x, y} } | ||
pairs | ||
end | ||
|
||
# Returns an `Array` of Tuples populated with the *N* element from `self` | ||
# and *N* element from `other`. |
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.
ditto from above
src/indexable.cr
Outdated
def zip(other : Indexable(U)) forall U | ||
pairs = Array({T, U}).new(size) | ||
zip(other) { |x, y| pairs << {x, y} } | ||
pairs | ||
end | ||
|
||
# Returns an `Array` of Tuples populated with the *N* element from `self` | ||
# and *N* element from `other`. | ||
# It adds *nil* when `other` returns outside the range for the given index. |
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.
If
other
has no index corresponding toself
(whenother
is shorter thanself
), the missing element is substituted withnil
.
src/indexable.cr
Outdated
# a = [1, 2, 3] | ||
# b = [4, 5] | ||
# | ||
# a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}] |
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.
{1, 4}, {2, 5}, {3, nil}
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.
Ops. 😬
src/indexable.cr
Outdated
def zip?(other : Indexable) | ||
each_with_index do |elem, i| | ||
yield elem, other[i]? | ||
end | ||
end | ||
|
||
# Calls the given block once for each index in `self`, passing `self` | ||
# and `other` index element as parameter. |
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.
Ditto from the first comment.
src/indexable.cr
Outdated
def zip?(other : Indexable) | ||
each_with_index do |elem, i| | ||
yield elem, other[i]? | ||
end | ||
end | ||
|
||
# Calls the given block once for each index in `self`, passing `self` | ||
# and `other` index element as parameter. | ||
# It adds *nil* when `other` returns outside the range for a index. |
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.
Adapt this to be similar to the phrase of the other zip?
method.
src/indexable.cr
Outdated
# a = {1, 2, 3} | ||
# b = {"a", "b"} | ||
# | ||
# a.zip?(b) { |x,y| puts "[ #{x} -- #{y} ]" } |
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.
ditto
src/indexable.cr
Outdated
@@ -412,24 +412,80 @@ module Indexable(T) | |||
indexes.map { |index| self[index] } | |||
end | |||
|
|||
# Calls the given block once for each index in `self`, passing `self` |
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.
`self` and `other` index element
is ambiguous and difficult to understand. Maybe `{self[i], other[i]}` for each index
?
src/indexable.cr
Outdated
# [ 1 -- a ] | ||
# [ 2 -- b ] | ||
# [ 3 -- c ] | ||
#``` | ||
def zip(other : Indexable) |
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.
For documentation purposes, I would prefer to have the block argument in the method signature even if it is not used as a captured block: This makes it clear that this method definition is expected to be called with a block and it shows the block types:
def zip(other : Indexable(U), &block : T, U ->) forall U
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.
👍 Completely agree with your point, it makes extremely more readable!
It also brings me a question:
The Indexable module is used as a mixin in other classes/structs what adds to them the zip
methods behavior in addition to others. Currently only Array#zip(?)
has tests covering it. Would make sense to move the specs to src/std/indexable_spec.cr
instead of src/std/array_spec.cr
?
src/indexable.cr
Outdated
# a.zip(b) { |x,y| puts "[ #{x} -- #{y} ]" } | ||
# ``` | ||
# | ||
# produces: |
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.
The example would be more condensed if t was in one code block and the printed output just commented lines.
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 is possible, I firstly did like you mentioned, but after some research, I followed the example what exists at src/dir.cr
to keep the consistency.
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.
I'm not sure consistency is that important for this and besides, we can change the example for Dir
as well...
It's just an unnecessary division between the example code and output. IMHO it should be as condensed as possible For examples that cover multiple files, I usually just print them in one code block and use comments to indicate different files. This keeps things compact and connected.
src/indexable.cr
Outdated
# b = ["a", "b", "c"] | ||
# | ||
# a.zip(b) # => [{1, "a"}, {2, "b"}, {3, "c"}] | ||
# ``` | ||
def zip(other : Indexable(U)) forall U |
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.
Also for documentation purposes, I would prefer to state the return type explicitly:
def zip(other : Indexable(U)) : Array({T, U}) forall U
src/indexable.cr
Outdated
def zip(other : Indexable) | ||
each_with_index do |elem, i| | ||
yield elem, other[i] | ||
end | ||
end | ||
|
||
# Returns an `Array` of Tuples populated with the *N* index |
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.
better: nth index
.
Or like above: `Array` of tuples `{self[i], other[i]}` for each index.
src/indexable.cr
Outdated
# Returns an `Array` of Tuples populated with the *N* index | ||
# element from `self` and *N* index element from `other`. | ||
# | ||
# Raises IndexError if `other` has no index corresponding to `self`. |
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.
Maybe Raises `IndexError` if `self` has more elements than `other`.
src/indexable.cr
Outdated
def zip(other : Indexable(U)) forall U | ||
pairs = Array({T, U}).new(size) | ||
zip(other) { |x, y| pairs << {x, y} } | ||
pairs | ||
end | ||
|
||
# Returns an `Array` of Tuples populated with the *N* element from `self` | ||
# and *N* element from `other`. | ||
# It adds *nil* when `other` returns outside the range for the given index. |
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.
This sentence doesn't really make sense. Perhaps: If `self` has more entries than `other`, missing values are filled up with `nil`.
src/indexable.cr
Outdated
# a = [1, 2, 3] | ||
# b = [4, 5] | ||
# | ||
# a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}] |
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.
This result is incorrect, with the given sample data it would be [{1, 4}, {2, 5}, {3, nil}]
. But it would probably be best to adjust the values in b
.
src/indexable.cr
Outdated
# b = [4, 5] | ||
# | ||
# a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}] | ||
# ``` | ||
def zip?(other : Indexable) |
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.
ditto:
def zip(other : Indexable(U), &block : T, U? ->) forall U
src/indexable.cr
Outdated
# [ 2 -- b ] | ||
# [ 3 -- ] | ||
# ``` | ||
# | ||
def zip?(other : Indexable(U)) forall U |
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.
ditto:
def zip(other : Indexable(U)) : Array({T, U?}) forall U
dc59ecf
to
94c189d
Compare
src/indexable.cr
Outdated
# | ||
# ``` | ||
# a = [1, 2, 3] | ||
# b = ["a", "b", "c"] | ||
# | ||
# a.zip(b) { |x,y| puts "[ #{x} -- #{y} ]" } | ||
# a.zip(b) { |x,y| puts "#{x} -- #{y}" } | ||
# => 1 --- 5 |
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.
this should be 1 -- a
and ditto.
src/indexable.cr
Outdated
# ``` | ||
def zip?(other : Indexable) | ||
def zip?(other : Indexable, &block : T, U ->) forall U |
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.
Indexable(U)
, then T, U? ->
src/indexable.cr
Outdated
def zip?(other : Indexable(U)) forall U | ||
# a.zip?(b) # => [{1, 4}, {2, 5}, {3, nil}] | ||
# ``` | ||
def zip?(other : Indexable(U) : Array({T, U})) forall U |
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.
The syntax is malformed here.
Also, should be Array({T, U?})
src/indexable.cr
Outdated
# | ||
# a.zip(b) # => [{1, "a"}, {2, "b"}, {3, "c"}] | ||
# ``` | ||
def zip(other : Indexable(U)) : Array({T, U?}) forall U |
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.
This should be Array({T, U})
still, since it's zip
not zip?
. THis is causing the compile errors.
src/indexable.cr
Outdated
# a.zip(b) # => [{1, "a"}, {2, "b"}, {3, "c"}] | ||
# ``` | ||
def zip(other : Indexable(U)) : Array({T, U?}) forall U | ||
pairs = Array({T, U?}).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.
ditto.
@RX14 tests are passing, if everything looks fine, I will squash the commits into a single one. |
86961b0
to
4666d56
Compare
src/indexable.cr
Outdated
# b = ["a", "b", "c"] | ||
# | ||
# a.zip(b) { |x,y| puts "#{x} -- #{y}" } | ||
# |
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.
The code block needs to be closed.
src/indexable.cr
Outdated
each_with_index do |elem, i| | ||
yield elem, other[i]? | ||
end | ||
end | ||
|
||
def zip?(other : Indexable(U)) forall U | ||
# Returns an `Array` of tuples populated with the _i_th indexed element from |
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.
You used *i*th
in the other method's doc.
format failure |
I think this needs a rebase onto master to fix CI |
6e2d3c2
to
135970f
Compare
@RX14 All green. 💚 I will squash it later to get it merged. |
135970f
to
c61d580
Compare
@RX14 squashed! |
Thanks @rodrigopinto ! |
This PR is for adding a documentation for
Indexable#zip
methods. It closes #5732.Indexable#zip(other : Indexable)
Indexable#zip(other : Indexable) &block
Indexable#zip?(other : Indexable)
Indexable#zip?(other : Indexable) &block