Add missing documentation to Hash.merge!(other : Hash, &block) #8380

jan-zajic · 2019-10-25T10:20:48Z

Completely missing description in api docs for Hash.merge! method, version with block. Without description, one cannot known which arguments are passed to given block (Except Ruby programmers, of course, because it behave same).
Related to #8377.

src/hash.cr

straight-shoota · 2019-10-25T11:02:23Z

src/hash.cr

@@ -1343,6 +1343,15 @@ class Hash(K, V)
    self
  end

+  # Adds the contents of *other* to this hash. 
+  # The value of each duplicate key is determined by calling the block with the key, its value in this hash and its value in *other*.


Maybe a little bit more verbose description, but help understanding:

Suggested change

# The value of each duplicate key is determined by calling the block with the key, its value in this hash and its value in *other*.

# If a key exists in both hashes, the given block is called to determine the value to be used. The block arguments are the key, the value in `self` and the value in *other*.

You're right i chaged it.

straight-shoota · 2019-10-25T11:04:15Z

src/hash.cr

+  # other = { "b" => 254, "c" => 300 }
+  # hash.merge!(other) { |key, v1, v2| v1 }
+  # hash # => {"a" => 100, "b" => 200, "c" => 300}
+  #```
  def merge!(other : Hash, &block)


You can also add documentation for the block types:

Suggested change

def merge!(other : Hash, &block)

def merge!(other : Hash, & : (K, V, V) -> V) : self

Are you sure? Types were specified, but removed in 3c17d07 (#1693). And it's sad for me, i read discussion here #5155 and say wow. It's seems like a very big bug for me - someone should fix this recursive types or it must be removed from lang, if they block clear type safety like here...

Wow, amazing that you digged up that old issue.

But anyway, even without recursive aliases, the type restriction is incorrect, because the generic arguments of other don't need to equal those of self, they can also be smaller.
For example you can merge Hash(String, Bool) into Hash(String | Int32, Bool | Nil). This can't be expressed currently, but is tracked in #3803. So, yeah let's leave it for now.

At least the new syntax for non-captured blocks can be used.

jan-zajic · 2019-10-25T15:24:31Z

And now i found that i forgot non bang version with block, def merge(other : Hash(L, W), &block : K, V, W -> V | W) forall L, W. But when i try it, i found maybe a bug, at least it's very confusing. It's documented to accept other hash with any types of keys and values, but block is defined to receive type of key K as in self, but if you look at implementation, it iterates second one so it can't compile at all.
Try for example this:

h1 = {"a" => 1, "b" => 2, "c" => 3}
h2 = {:a => 1, :b => 2, :c => 3}
h3 = h1.merge(h2){|k,v1,v2| v1||v2}

which leads to:
Error: argument #1 of yield expected to be String, not Symbol
In my opinion it must be reimplemented or changed to: def merge(other : Hash(K, W), &block : K, V, W -> V | W) forall W
Than we should add documentation to it.

RX14 · 2019-10-31T17:47:10Z

@jan-zajic how about

def merge(other : Hash(K2, V2), & : (K | K2, V, V2) -> V | V2) forall K2, V2

?

RX14

Thank you! This is already a strict improvement to the documentation.

Blacksmoke16 · 2019-10-31T17:55:36Z

src/hash.cr

@@ -1343,7 +1343,17 @@ class Hash(K, V)
    self
  end

-  def merge!(other : Hash, &block)
+  # Adds the contents of *other* to this hash.


Suggested change

# Adds the contents of *other* to this hash.

# Adds the contents of *other* to `self`.

straight-shoota · 2019-10-31T18:24:48Z

@RX14

def merge(other : Hash(K2, V2), & : (K | K2, V, V2) -> V | V2) forall K2, V2

That would work, but I think it's to complicated. The type restriction is not necessary for technical reasons, but mainly for documentation. For improving the documentation, this looks too complex IMO. And it's confusing because it's not really for all K2, V2. They must be covariant with K, V.

Only benefit would be more precise error messages when using this method with non-matching types.

Btw. the key in the block argument is not K | K2 but only K because only keys from self are yielded.

RX14 · 2019-11-08T11:25:58Z

They must be covariant with K, V.

Ah, I didn't look at the implementation....

class Hash(K, V)
  def merge2(other : Hash(K2, V2), & : (K2, V, V2) -> V | V2) : Hash(K | K2, V | V2) forall K2, V2
    hash = Hash(K | K2, V | V2).new
    hash.merge! self
    
    other.each do |k2, v2|
      if self.has_key?(k2)
        hash[k2] = yield k2, self[k2], v2
      else
        hash[k2] = v2
      end
    end
  
    hash
  end
end

This allows for the full type signature.

That's for another PR though...

That being said, I'd very much like the full type signature in the docs. That type signature encodes a lot of information, for example it's clear to see that the block is encoding a choice when you see V, V2 -> V | V2.

Plus it's actually K2 which is yielded.

RX14

Shall we merge?

asterite

Great, thank you!

straight-shoota · 2019-11-08T12:13:59Z

@RX14 Your code looks good. But that's actually #merge not #merge!. And it matches its current implementation 😄

RX14 · 2019-11-08T13:28:47Z

@straight-shoota We were talking about the non-bang version the whole time! I was responding to @jan-zajic's comment about the non-bang version. Also, my implementation is different to the current implementation in that it uses self[k2] instead of hash[k2] which restricts the key type sent to the block from K | K2 to K2.

jan-zajic · 2019-11-08T13:48:16Z

@straight-shoota @RX14 yes that #8380 (comment) comment ;). Well that this is already merged. We can continue that discussion in some issue or in the forum ...

RX14 · 2019-11-08T14:26:10Z

Yeah, I was planning to make this another PR.

straight-shoota · 2019-11-08T14:55:43Z

Ooops, I think I got lost somewehere.

straight-shoota reviewed Oct 25, 2019

View reviewed changes

add missing documentation to Hash.merge!(other : Hash, &block)

df48642

jan-zajic force-pushed the feature/hashMergeDoc branch from 6d095e0 to df48642 Compare October 25, 2019 15:13

RX14 approved these changes Oct 31, 2019

View reviewed changes

RX14 added kind:docs topic:stdlib:collection labels Oct 31, 2019

Blacksmoke16 reviewed Oct 31, 2019

View reviewed changes

RX14 approved these changes Nov 8, 2019

View reviewed changes

asterite approved these changes Nov 8, 2019

View reviewed changes

asterite merged commit e09bdce into crystal-lang:master Nov 8, 2019

asterite added this to the 0.32.0 milestone Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing documentation to Hash.merge!(other : Hash, &block) #8380

Add missing documentation to Hash.merge!(other : Hash, &block) #8380

jan-zajic commented Oct 25, 2019

straight-shoota Oct 25, 2019

jan-zajic Oct 25, 2019

straight-shoota Oct 25, 2019

jan-zajic Oct 25, 2019 •

edited

Loading

straight-shoota Oct 25, 2019

j8r Oct 25, 2019

jan-zajic commented Oct 25, 2019

RX14 commented Oct 31, 2019 •

edited

Loading

RX14 left a comment

Blacksmoke16 Oct 31, 2019

straight-shoota commented Oct 31, 2019 •

edited

Loading

RX14 commented Nov 8, 2019 •

edited

Loading

RX14 left a comment

asterite left a comment

straight-shoota commented Nov 8, 2019

RX14 commented Nov 8, 2019

jan-zajic commented Nov 8, 2019

RX14 commented Nov 8, 2019

straight-shoota commented Nov 8, 2019

	# The value of each duplicate key is determined by calling the block with the key, its value in this hash and its value in other.
	# If a key exists in both hashes, the given block is called to determine the value to be used. The block arguments are the key, the value in `self` and the value in other.

	def merge!(other : Hash, &block)
	def merge!(other : Hash, & : (K, V, V) -> V) : self

	# Adds the contents of other to this hash.
	# Adds the contents of other to `self`.

Add missing documentation to Hash.merge!(other : Hash, &block) #8380

Add missing documentation to Hash.merge!(other : Hash, &block) #8380

Conversation

jan-zajic commented Oct 25, 2019

straight-shoota Oct 25, 2019

Choose a reason for hiding this comment

jan-zajic Oct 25, 2019

Choose a reason for hiding this comment

straight-shoota Oct 25, 2019

Choose a reason for hiding this comment

jan-zajic Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

straight-shoota Oct 25, 2019

Choose a reason for hiding this comment

j8r Oct 25, 2019

Choose a reason for hiding this comment

jan-zajic commented Oct 25, 2019

RX14 commented Oct 31, 2019 • edited Loading

RX14 left a comment

Choose a reason for hiding this comment

Blacksmoke16 Oct 31, 2019

Choose a reason for hiding this comment

straight-shoota commented Oct 31, 2019 • edited Loading

RX14 commented Nov 8, 2019 • edited Loading

RX14 left a comment

Choose a reason for hiding this comment

asterite left a comment

Choose a reason for hiding this comment

straight-shoota commented Nov 8, 2019

RX14 commented Nov 8, 2019

jan-zajic commented Nov 8, 2019

RX14 commented Nov 8, 2019

straight-shoota commented Nov 8, 2019

jan-zajic Oct 25, 2019 •

edited

Loading

RX14 commented Oct 31, 2019 •

edited

Loading

straight-shoota commented Oct 31, 2019 •

edited

Loading

RX14 commented Nov 8, 2019 •

edited

Loading