Documenting `Indexable#zip` and `Indexable#zip?` methods. #5734

rodrigopinto · 2018-02-20T19:33:49Z

This PR is for adding a documentation for Indexable#zip methods. It closes #5732.

Add documentation to Indexable#zip(other : Indexable)
Add documentation to Indexable#zip(other : Indexable) &block
Add documentation to Indexable#zip?(other : Indexable)
Add documentation to Indexable#zip?(other : Indexable) &block

RX14 · 2018-02-20T21:20:24Z

src/indexable.cr

@@ -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`


Calls the given block for each index in self, passing in the element at that index in self and the element at the same index in other.

?

Also make sure to note that this method raises.

RX14 · 2018-02-20T21:21:24Z

src/indexable.cr

+  # a = [1, 2, 3]
+  # b = ["a", "b", "c"]
+  #
+  # a.zip(b)  { |x,y| puts "[ #{x} -- #{y} ]" }


just "#{x} -- #{y}"? Not sure why we need the [].

RX14 · 2018-02-20T21:24:26Z

src/indexable.cr

  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`.


Returns an Array of tuples populated with the ith indexed element from self followed by the ith indexed element from other, for all i 0..self.size - 1

Maybe that needs more tweaking.

RX14 · 2018-02-20T21:24:48Z

src/indexable.cr

+  # 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`.


I'd mention that this happens when other is shorter than self.

Makes sense. It makes the sentence more clear.

RX14 · 2018-02-20T21:25:23Z

src/indexable.cr

  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`.


ditto from above

RX14 · 2018-02-20T21:26:33Z

src/indexable.cr

  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.


If other has no index corresponding to self (when other is shorter than self), the missing element is substituted with nil.

RX14 · 2018-02-20T21:27:10Z

src/indexable.cr

+  # a = [1, 2, 3]
+  # b = [4, 5]
+  #
+  # a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}]


{1, 4}, {2, 5}, {3, nil}

RX14 · 2018-02-20T21:27:35Z

src/indexable.cr

  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.


Ditto from the first comment.

RX14 · 2018-02-20T21:27:57Z

src/indexable.cr

  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.


Adapt this to be similar to the phrase of the other zip? method.

RX14 · 2018-02-20T21:28:05Z

src/indexable.cr

+  # a = {1, 2, 3}
+  # b = {"a", "b"}
+  #
+  # a.zip?(b) { |x,y| puts "[ #{x} -- #{y} ]" }


straight-shoota · 2018-02-20T21:28:51Z

src/indexable.cr

@@ -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`


`self` and `other` index element is ambiguous and difficult to understand. Maybe `{self[i], other[i]}` for each index?

straight-shoota · 2018-02-20T21:32:03Z

src/indexable.cr

+  # [ 1 -- a ]
+  # [ 2 -- b ]
+  # [ 3 -- c ]
+  #```
  def zip(other : Indexable)


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

👍 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?

straight-shoota · 2018-02-20T21:33:29Z

src/indexable.cr

+  # a.zip(b)  { |x,y| puts "[ #{x} -- #{y} ]" }
+  # ```
+  #
+  # produces:


The example would be more condensed if t was in one code block and the printed output just commented lines.

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.

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.

straight-shoota · 2018-02-20T21:34:55Z

src/indexable.cr

+  # b = ["a", "b", "c"]
+  #
+  # a.zip(b) # => [{1, "a"}, {2, "b"}, {3, "c"}]
+  # ```
  def zip(other : Indexable(U)) forall U


Also for documentation purposes, I would prefer to state the return type explicitly:

def zip(other : Indexable(U)) : Array({T, U}) forall U

straight-shoota · 2018-02-20T21:37:16Z

src/indexable.cr

  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


better: nth index.
Or like above: `Array` of tuples `{self[i], other[i]}` for each index.

straight-shoota · 2018-02-20T21:39:43Z

src/indexable.cr

+  # 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`.


Maybe Raises `IndexError` if `self` has more elements than `other`.

straight-shoota · 2018-02-20T21:41:06Z

src/indexable.cr

  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.


This sentence doesn't really make sense. Perhaps: If `self` has more entries than `other`, missing values are filled up with `nil`.

straight-shoota · 2018-02-20T21:42:02Z

src/indexable.cr

+  # a = [1, 2, 3]
+  # b = [4, 5]
+  #
+  # a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}]


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.

straight-shoota · 2018-02-20T21:42:57Z

src/indexable.cr

+  # b = [4, 5]
+  #
+  # a.zip(b) # => [{1, 1}, {2, 2}, {3, nil}]
+  # ```
  def zip?(other : Indexable)


ditto:

def zip(other : Indexable(U), &block : T, U? ->) forall U

straight-shoota · 2018-02-20T21:43:55Z

src/indexable.cr

+  # [ 2 -- b ]
+  # [ 3 --  ]
+  # ```
+  #
  def zip?(other : Indexable(U)) forall U


ditto:

def zip(other : Indexable(U)) : Array({T, U?}) forall U

RX14 · 2018-02-25T13:30:07Z

src/indexable.cr

  #
  # ```
  # 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


this should be 1 -- a and ditto.

RX14 · 2018-02-25T13:33:02Z

src/indexable.cr

  # ```
-  def zip?(other : Indexable)
+  def zip?(other : Indexable, &block : T, U ->) forall U


Indexable(U), then T, U? ->

RX14 · 2018-02-25T13:33:49Z

src/indexable.cr

-  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


The syntax is malformed here.

Also, should be Array({T, U?})

Oops, looks like I reviewed an old diff

RX14 · 2018-02-25T13:39:47Z

src/indexable.cr

+  #
+  # a.zip(b) # => [{1, "a"}, {2, "b"}, {3, "c"}]
+  # ```
+  def zip(other : Indexable(U)) : Array({T, U?}) forall U


This should be Array({T, U}) still, since it's zip not zip?. THis is causing the compile errors.

RX14 · 2018-02-25T13:39:52Z

src/indexable.cr

+  # 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)


rodrigopinto · 2018-02-25T22:22:01Z

@RX14 tests are passing, if everything looks fine, I will squash the commits into a single one.

straight-shoota · 2018-04-04T21:26:00Z

src/indexable.cr

+  # b = ["a", "b", "c"]
+  #
+  # a.zip(b)  { |x,y| puts "#{x} -- #{y}" }
+  #


The code block needs to be closed.

straight-shoota · 2018-04-04T21:28:05Z

src/indexable.cr

    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


You used *i*th in the other method's doc.

RX14 · 2018-06-08T15:37:02Z

format failure

RX14 · 2018-06-10T13:27:22Z

I think this needs a rebase onto master to fix CI

rodrigopinto · 2018-06-11T06:59:40Z

@RX14 All green. 💚 I will squash it later to get it merged.

rodrigopinto · 2018-06-13T17:38:26Z

@RX14 squashed!

bcardiff · 2018-06-13T19:43:51Z

Thanks @rodrigopinto !

RX14 requested changes Feb 20, 2018

View reviewed changes

straight-shoota requested changes Feb 20, 2018

View reviewed changes

rodrigopinto force-pushed the rp-add-docs-to-indexable-zip-methods branch from dc59ecf to 94c189d Compare February 24, 2018 09:06

RX14 previously requested changes Feb 25, 2018

View reviewed changes

RX14 requested changes Feb 25, 2018

View reviewed changes

RX14 approved these changes Feb 25, 2018

View reviewed changes

rodrigopinto force-pushed the rp-add-docs-to-indexable-zip-methods branch from 86961b0 to 4666d56 Compare February 25, 2018 22:34

straight-shoota reviewed Apr 4, 2018

View reviewed changes

sdogruyol added the kind:docs label Apr 12, 2018

rodrigopinto force-pushed the rp-add-docs-to-indexable-zip-methods branch from 6e2d3c2 to 135970f Compare June 10, 2018 18:07

RX14 approved these changes Jun 11, 2018

View reviewed changes

Documenting Indexable#zip and Indexable#zip? methods.

c61d580

rodrigopinto force-pushed the rp-add-docs-to-indexable-zip-methods branch from 135970f to c61d580 Compare June 13, 2018 17:37

bcardiff merged commit bb89684 into crystal-lang:master Jun 13, 2018

bcardiff added this to the 0.25.1 milestone Jun 13, 2018

rodrigopinto deleted the rp-add-docs-to-indexable-zip-methods branch June 14, 2018 04:57

Documenting Indexable#zip and Indexable#zip? methods. #5734

Documenting Indexable#zip and Indexable#zip? methods. #5734

Conversation

rodrigopinto commented Feb 20, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigopinto commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Jun 8, 2018

RX14 commented Jun 10, 2018

rodrigopinto commented Jun 11, 2018

rodrigopinto commented Jun 13, 2018

bcardiff commented Jun 13, 2018

Documenting `Indexable#zip` and `Indexable#zip?` methods. #5734

Documenting `Indexable#zip` and `Indexable#zip?` methods. #5734