Skip to content

Loading…

ActiveSupport: Added `each_with_hash` to Enumerable #8088

Closed
wants to merge 1 commit into from

4 participants

@ndbroadbent

This is in response to the discussion at https://bugs.ruby-lang.org/issues/7241

I think an each_with_hash method would be a very useful addition. It provides a nice shortcut for the common each_with_object({}), but it also supports setting a default value or proc for the Hash. It also provides a shortcut for a common use of Hash.new( <proc> ), which is setting the default value on a hash key when it is looked up. Here's an example of how you could write less code using that feature:

# using each_with_object:

[1,2,3,2].each_with_object(Hash.new(&-> hash, key { hash[key] = [] })) {|el, hash| hash[el] << el ** 2 }
#=> {1=>[1], 2=>[4, 4], 3=>[9]}

# using each_with_hash:

[1,2,3,2].each_with_hash([], true) {|hash, el| hash[el] << el ** 2}
#=> {1=>[1], 2=>[4, 4], 3=>[9]}

I would love to be able to easily use << with arrays, instead of having to write hash[el] += [el ** 2].

If this gets merged, it would be awesome if it could also be included in 3-2-stable.

Thanks!

@carlosantoniodasilva
Ruby on Rails member

Thanks for your contribution.

I'm :-1: on adding this to Active Support. I'm quite sure Rails code won't benefit from it, and it adds another layer of indirection for end users of the framework. Besides that, writing less code not always shows better intention, in some scenarios having a couple of variables with good names will for sure make the code easier to maintain. Thanks!

@ndbroadbent

Thanks for your feedback! I've updated the pull request to show where each_with_hash would be useful in the Rails codebase.

@ndbroadbent ndbroadbent commented on the diff
activerecord/lib/active_record/model_schema.rb
@@ -258,7 +258,7 @@ def content_columns
# and true as the value. This makes it possible to do O(1) lookups in respond_to? to check if a given method for attribute
# is available.
def column_methods_hash #:nodoc:
- @dynamic_methods_hash ||= column_names.each_with_object(Hash.new(false)) do |attr, methods|
+ @dynamic_methods_hash ||= column_names.each_with_hash(false) do |attr, methods|

This change in particular shows how each_with_hash would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ndbroadbent ndbroadbent Added 'each_with_hash' to Enumerable
Replaced instances of `each_with_object({})`.
6fb26f0
@carlosantoniodasilva
Ruby on Rails member

Thanks for the update. What I meant to say was that it's probably a feature that would not be used inside Rails code, even if Active Support had it and some places could make use of it, mainly to avoid that level of indirection and calling the extra method.

@ndbroadbent

OK, I can understand that. Well, I'll wait to see if Matz likes the idea, and whether or not it can be accepted into Ruby.

@sj26

Sorry, but I don't like this. While this is a popular patten, adding this layer of indirection doesn't seem useful to me. The pattern is easily achievable using each_with_object, and overloading the defaults behaviour of hash actually introduces confusion for me—I had to read the implementation to verify you were duping the default value like in your stated example with the array, and only when a boolean is passed, otherwise you'd get the same default instance.

I don't see the useful difference between

[1, 2, 2, 3].each_with_object(Hash.new { |hash, key| hash[key] = key }) { |value, hash| hash[value] **= 2 }

and

[1, 2, 2, 3].each_with_hash(proc { |hash, key| hash[key] = key }) { |value, hash| hash[value] **= 2 }

and I find this just plain old confusing:

[1, 2, 2, 3].each_with_hash(proc { |hash, key| key }, true) { |value, hash| hash[value] **= 2 }

what does true mean? A bare positional parameter like this usually introduces confusion.

Additionally, this breaks two convention introduced by each_with_object: the order of parameters is reversed (el, object vs hash, el), and it's not chainable (i.e. .with_object on an enumerator), but these would be easily fixed.

@ndbroadbent

Hi @sj26, thanks for your feedback! The boolean argument is inspired by Ruby's respond_to? method, which has a similar method signature. I understand if people find that more confusing than useful, but I think it's pretty cool.

Happy to remove that part though, I think the PR is still useful enough without it.

@jeremy
Ruby on Rails member

I appreciate the motivation, too, but agree that it actually obscures what's happening. Creating the hash first then iterating with it isn't a one-liner, but it's clearer! Thanks @ndbroadbent :)

@jeremy jeremy closed this
@ndbroadbent

Sweet, no worries! Was worth a shot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 1, 2012
  1. @ndbroadbent

    Added 'each_with_hash' to Enumerable

    ndbroadbent committed
    Replaced instances of `each_with_object({})`.
View
2 actionpack/lib/action_dispatch/request/session.rb
@@ -168,7 +168,7 @@ def load!
end
def stringify_keys(other)
- other.each_with_object({}) { |(key, value), hash|
+ other.each_with_hash { |(key, value), hash|
hash[key.to_s] = value
}
end
View
2 actionpack/lib/action_view/renderer/abstract_renderer.rb
@@ -13,7 +13,7 @@ def render
protected
def extract_details(options)
- @lookup_context.registered_details.each_with_object({}) do |key, details|
+ @lookup_context.registered_details.each_with_hash do |key, details|
next unless value = options[key]
details[key] = Array(value)
end
View
2 activerecord/lib/active_record/attribute_methods.rb
@@ -201,7 +201,7 @@ def attribute_names
# person.attributes
# # => {"id"=>3, "created_at"=>Sun, 21 Oct 2012 04:53:04, "updated_at"=>Sun, 21 Oct 2012 04:53:04, "name"=>"Francesco", "age"=>22}
def attributes
- attribute_names.each_with_object({}) { |name, attrs|
+ attribute_names.each_with_hash { |name, attrs|
attrs[name] = read_attribute(name)
}
end
View
2 activerecord/lib/active_record/model_schema.rb
@@ -258,7 +258,7 @@ def content_columns
# and true as the value. This makes it possible to do O(1) lookups in respond_to? to check if a given method for attribute
# is available.
def column_methods_hash #:nodoc:
- @dynamic_methods_hash ||= column_names.each_with_object(Hash.new(false)) do |attr, methods|
+ @dynamic_methods_hash ||= column_names.each_with_hash(false) do |attr, methods|

This change in particular shows how each_with_hash would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
attr_name = attr.to_s
methods[attr.to_sym] = attr_name
methods["#{attr}=".to_sym] = attr_name
View
2 activerecord/lib/active_record/relation/query_methods.rb
@@ -808,7 +808,7 @@ def reverse_sql_order(order_query)
when Symbol
{ o => :desc }
when Hash
- o.each_with_object({}) do |(field, dir), memo|
+ o.each_with_hash do |(field, dir), memo|
memo[field] = (dir == :asc ? :desc : :asc )
end
else
View
34 activesupport/lib/active_support/core_ext/enumerable.rb
@@ -39,6 +39,40 @@ def index_by
end
end
+ # Convert an enumerable to a hash, with the same block syntax as <tt>each_with_object</tt>.
+ # Allows a default value or proc to be given. If <tt>set_default_on_lookup</tt> is <tt>true</tt>,
+ # the default value will be converted to a proc that sets the Hash key when looked up.
+ # This is ignored if default is already a proc.
+ #
+ # [1,3,5].each_with_hash {|i, h| h[i] = i ** 2 }
+ # => {1=>1, 3=>9, 5=>25}
+ #
+ # [3,4,5,4].each_with_hash(0) {|i, h| h[i] += i }
+ # => {3=>3, 4=>8, 5=>5}
+ #
+ # ['red', 'blue'].each_with_hash(-> h, k { h[k] = k.upcase }) {|c, h| h[c] << '!' }
+ # => {'red'=>'RED!', 'blue'=>'BLUE!'}
+ #
+ # [1,2,3,2,1].each_with_hash([], true) {|i, h| h[i] << i * 2 }
+ # => {1=>[2, 2], 2=>[4, 4], 3=>[3]}
+ def each_with_hash(default = nil, set_default_on_lookup = false)
+ if block_given?
+ hash = if Proc === default
+ Hash.new &default
+ else
+ if set_default_on_lookup
+ Hash.new &-> hash, key { hash[key] = default.dup }
+ else
+ Hash.new default
+ end
+ end
+ self.each { |el| yield el, hash }
+ hash
+ else
+ to_enum :each_with_hash, default, set_default_on_lookup
+ end
+ end
+
# Returns +true+ if the enumerable has more than 1 element. Functionally
# equivalent to <tt>enum.to_a.size > 1</tt>. Can be called with a block too,
# much like any?, so <tt>people.many? { |p| p.age > 26 }</tt> returns +true+
View
54 activesupport/test/core_ext/enumerable_test.rb
@@ -93,11 +93,55 @@ def test_range_sums
def test_index_by
payments = GenericEnumerable.new([ Payment.new(5), Payment.new(15), Payment.new(10) ])
- assert_equal({ 5 => Payment.new(5), 15 => Payment.new(15), 10 => Payment.new(10) },
- payments.index_by { |p| p.price })
- assert_equal Enumerator, payments.index_by.class
- assert_equal({ 5 => Payment.new(5), 15 => Payment.new(15), 10 => Payment.new(10) },
- payments.index_by.each { |p| p.price })
+ expected_result = { 5 => Payment.new(5), 15 => Payment.new(15), 10 => Payment.new(10) }
+ assert_equal expected_result, payments.index_by { |p| p.price }
+ assert_equal Enumerator, payments.index_by.class
+ assert_equal expected_result, payments.index_by.each { |p| p.price }
+ end
+
+ def test_each_with_hash
+ payments = GenericEnumerable.new([ Payment.new(5), Payment.new(15), Payment.new(10) ])
+
+ block = -> p, hash { hash[p.price] = p.price * 2 }
+
+ expected_result = { 5 => 10, 15 => 30, 10 => 20 }
+ assert_equal expected_result, payments.each_with_hash(&block)
+ assert_equal Enumerator, payments.each_with_hash.class
+ assert_equal expected_result, payments.each_with_hash.each(&block)
+ end
+
+ def test_each_with_hash_default
+ payments = GenericEnumerable.new([ Payment.new(5), Payment.new(15), Payment.new(10) ])
+
+ block = -> p, hash do
+ hash[:total_price] += p.price
+ hash[:total_payments] += 1
+ end
+ expected_result = { :total_price => 30, :total_payments => 3 }
+ assert_equal expected_result, payments.each_with_hash(0, &block)
+ assert_equal expected_result, payments.each_with_hash(0).each(&block)
+ end
+
+ def test_each_with_hash_set_default_on_lookup
+ payments = GenericEnumerable.new([ Payment.new(5), Payment.new(15), Payment.new(10) ])
+ expected_result = {
+ :payments_gt_10 => [Payment.new(15)],
+ :payments_lte_10 => [Payment.new(5), Payment.new(10)]
+ }
+ result = payments.each_with_hash([], true) do |p, hash|
+ key = p.price <= 10 ? :payments_lte_10 : :payments_gt_10
+ hash[key] << p
+ end
+ assert_equal expected_result, result
+ end
+
+ def test_each_with_hash_default_proc
+ colors = GenericEnumerable.new([ "red", "blue", "green" ])
+
+ result = colors.each_with_hash(-> h, k { h[k] = k.upcase }) do |c, hash|
+ hash[c] << '!'
+ end
+ assert_equal({ "red" => "RED!", "blue" => "BLUE!", "green" => "GREEN!" }, result)
end
def test_many
Something went wrong with that request. Please try again.