Make the number of arguments of `Hash#merge` variable #1951

liwii · 2018-09-13T05:55:00Z

Abstract

Make the number of arguments of Hash#merge variable.

Background

In many websites such as Stack Overflow and Qiita, many people are seeking how to merge more than three hashes.

https://stackoverflow.com/questions/19548496/how-to-merge-multiple-hashes-in-ruby
https://qiita.com/hc0208/items/c662f5189fa383872f4e
https://stackoverrun.com/ja/q/4997431

Many ways, like using Enumerable#inject or calling Hash#merge multiple times, are proposed, but both don't seem intuitive. Especially when using block in Hash#merge, the code becomes too complicated.

Proposal

Change the argument of Hash#merge from singular to variable length.

Implementation

The code below.

Evaluation

The code to merge more than three hashes became much simpler and more intuitive.

before

hash1.merge(hash2).merge(hash3)

[hash1, hash2, hash3].inject do |result, part|
  result.merge(part) { |key, value1, value2| key + value1 + value2 }
end

after

hash1.merge(hash2, hash3)
hash1.merge(hash2, hash3) { |key, value1, value2| key + value1 + value2 }

Discussion

Summary

The change is needed to make Hash#merge more useful and intuitive.

knu

You also need to update the documentation, and clarify that the method is allowed to be called with zero argument by adding a test case.

knu · 2018-09-14T06:07:23Z

hash.c

-    if (rb_block_given_p()) {
-	rb_hash_foreach(hash2, rb_hash_update_block_i, hash1);
+    rb_hash_modify(self);
+    for(int i = 0; i < argc; i++){


CRuby is written in C90, so you should stick to the old syntax here.

int i; rb_hash_modify(self); for (i = 0; i < argc; i++) {

knu · 2018-09-14T06:12:20Z

hash.c

+    rb_hash_modify(self);
+    for(int i = 0; i < argc; i++){
+      VALUE hash = to_hash(argv[i]);
+      if (rb_block_given_p()) {


Maybe you can memoize this value.

knu · 2018-09-14T06:17:31Z

This proposal was accepted in yesterday's meeting, congratulations!

liwii · 2018-09-15T14:55:21Z

@knu
Thanks for your review! I modified codes following the review, and added documents and test cases.

nobu · 2018-09-15T15:25:57Z

hash.c

+    int i;
+
+    rb_hash_modify(self);
+    bool block_given = rb_block_given_p();


Variable declarations are not allowed after a statement.

nobu · 2018-09-15T15:27:37Z

hash.c

+ *  More than one hash can be given to this method as arguments, then
+ *  the method returns a new hash containing the contents of the reciever
+ *  itself and all hashes given as arguments. The method also can be called 
+ *  with no argument, then the receiver itself will be returned;


The result is a new hash, not the receiver itself.

liwii · 2018-09-15T16:22:19Z

@nobu
Thanks for your review. I fixed the problems you pointed out.

marcandre · 2018-09-16T03:33:11Z

Nice. As far as documentation goes, I'd keep it simple. I don't think that discussing the trivial case of no argument is necessary. The interface should be kept to a minimum by basically changing hsh.merge!(other_hash) to hsh.merge!(other_hash, another_hash, ...). See Array#concat for example.

liwii · 2018-09-16T13:44:12Z

@marcandre
Thanks for your review! I fixed documentation and simplified it. I'm not confident in English, so please tell me if you find something wrong in the documentation.

nobu · 2018-09-16T15:06:34Z

hash.c

+    bool block_given = rb_block_given_p();
+
+    rb_hash_modify(self);
+    for(i = 0; i < argc; i++){


The indentation and style in block differ than others.

simi · 2018-09-16T16:23:38Z

test/ruby/test_hash.rb

@@ -1119,15 +1119,40 @@ def test_hash2
  def test_update2
    h1 = @cls[1=>2, 3=>4]
    h2 = {1=>3, 5=>7}
+    h3 = {1=>1, 2=>4}


This variable is not used AFAIK.

liwii · 2018-09-17T02:35:55Z

@nobu
Thanks for review. I didn't notice it. I fixed the indentation in the new commit.

liwii · 2018-09-17T02:36:36Z

@simi
Thanks for review. It's completely my mistake. I've already fixed it.

mame · 2018-09-18T06:36:05Z

hash.c

 *
 *     h1 = { "a" => 100, "b" => 200 }
 *     h2 = { "b" => 254, "c" => 300 }
+ *     h3 = { "b" => 100, "d" => 400 }


Not needed?

liwii · 2018-09-19T00:20:09Z

@mame
You are right. It's needless. I fixed it.

marcandre · 2018-09-19T01:48:35Z

@marcandre
Thanks for your review! I fixed documentation and simplified it. I'm not confident in English, so please tell me if you find something wrong in the documentation.

Looks good. 👍 I'd even remove the part stating "The method also can be called with no argument, then nothing will change in the receiver.". It's important to test it, but it's not something people will use in general and should be obvious anyways.

k0kubun · 2018-09-19T02:08:45Z

hash.c

+       }
+       else {
+          rb_hash_foreach(hash, rb_hash_update_i, self);
+       }


very trivial: These lines are indented with 3 spaces from for loop, but maybe usually it's indented with 4 spaces

Fixed. Thank you.

mame · 2018-09-19T02:08:56Z

@liwii Merged! Thank you, and I expect you to continue contribution to Ruby 😃
@marcandre Oops, sorry I missed your comment. I agree with you. I've removed the sentence. Thank you.

As per Marc-Andre's comment. [Refs GH-1951] git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64779 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

marcandre · 2018-09-19T02:14:19Z

@marcandre Oops, sorry I missed your comment. I agree with you. I've removed the sentence.

Yes, I commented a few minutes before your commit. Cool, thanks! I added a note in the NEWS.

Congrats @liwii 🎉

… and `update` methods It follows Ruby 2.6 addition ruby/ruby#1951

liwii added 3 commits September 13, 2018 14:04

Make the number of arguments of variable

cd6b8b7

Make the test of the case in which no argument is given

f83e174

modify indent

c443195

knu requested changes Sep 14, 2018

View reviewed changes

Add documentation and test case, remove redundant statements

767c061

nobu reviewed Sep 15, 2018

View reviewed changes

Fix document and correct variable declaration

8a470b7

Fix documentation

d187ec6

nobu reviewed Sep 16, 2018

View reviewed changes

simi reviewed Sep 16, 2018

View reviewed changes

Fix indentation and unused variable in a test case

b3c1d84

knu approved these changes Sep 17, 2018

View reviewed changes

simi approved these changes Sep 17, 2018

View reviewed changes

mame reviewed Sep 18, 2018

View reviewed changes

hash.c Outdated

*

* h1 = { "a" => 100, "b" => 200 }

* h2 = { "b" => 254, "c" => 300 }

* h3 = { "b" => 100, "d" => 400 }

Copy link

Member

mame Sep 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed?

remove needless part of the document

8afc500

matzbot closed this in 085f5ef Sep 19, 2018

k0kubun reviewed Sep 19, 2018

View reviewed changes

matzbot pushed a commit that referenced this pull request Sep 19, 2018

* hash.c (rb_hash_update): remove a meticulous explanation

bfbf9bd

As per Marc-Andre's comment. [Refs GH-1951] git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64779 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

morgoth mentioned this pull request Aug 7, 2019

Support multiple arguments in HashWithIndifferentAccess for merge and update methods rails/rails#36880

Merged

morgoth added a commit to freeletics/rails that referenced this pull request Aug 7, 2019

Support multiple arguments in HashWithIndifferentAccess for merge…

8674417

… and `update` methods It follows Ruby 2.6 addition ruby/ruby#1951

hsbt added the Backport label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the number of arguments of `Hash#merge` variable #1951

Make the number of arguments of `Hash#merge` variable #1951

liwii commented Sep 13, 2018 •

edited

Loading

knu left a comment

knu Sep 14, 2018

knu Sep 14, 2018

knu commented Sep 14, 2018

liwii commented Sep 15, 2018

nobu Sep 15, 2018

nobu Sep 15, 2018

liwii commented Sep 15, 2018

marcandre commented Sep 16, 2018

liwii commented Sep 16, 2018

nobu Sep 16, 2018

simi Sep 16, 2018

liwii commented Sep 17, 2018

liwii commented Sep 17, 2018

mame Sep 18, 2018

liwii commented Sep 19, 2018

marcandre commented Sep 19, 2018

k0kubun Sep 19, 2018

mame Sep 19, 2018

mame commented Sep 19, 2018

marcandre commented Sep 19, 2018

Make the number of arguments of Hash#merge variable #1951

Make the number of arguments of Hash#merge variable #1951

Conversation

liwii commented Sep 13, 2018 • edited Loading

Abstract

Background

Proposal

Implementation

Evaluation

Discussion

Summary

knu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knu commented Sep 14, 2018

liwii commented Sep 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liwii commented Sep 15, 2018

marcandre commented Sep 16, 2018

liwii commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liwii commented Sep 17, 2018

liwii commented Sep 17, 2018

Choose a reason for hiding this comment

liwii commented Sep 19, 2018

marcandre commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mame commented Sep 19, 2018

marcandre commented Sep 19, 2018

Make the number of arguments of `Hash#merge` variable #1951

Make the number of arguments of `Hash#merge` variable #1951

liwii commented Sep 13, 2018 •

edited

Loading