Feature #16252: Hash#partition should return hashes

• Status: Open
• Priority: Normal
• Assignee: -
• Target version: -

[Dan0042 (Daniel DeLorme)]

Hash#partition is implemented by Enumerable so it just returns two arrays of arrays

{1=>2,3=>4}.partition{|k,|k==1} #=> [[[1, 2]], [[3, 4]]]

But I think it would make more sense to behave similarly to Hash#select and Hash#reject

{1=>2,3=>4}.partition{|k,|k==1} #=> [{1=>2}, {3=>4}]

[Dan0042 (Daniel DeLorme)]

Two years later, and today when using hash.partition I was surprised that it didn't return two hashes.
This has to be just an oversight, right?

[zverok (Victor Shepelev)]

I am afraid we don't have some particular rules for Enumerables to return their own class:

Set[*1..8].select(&:odd?)
# => [1, 3, 5, 7] -- not a Set

Making Hash#select and #reject return hashes was an ad-hoc decision (and quite useful at that!); maybe it would be good to consistently follow this decision for some core collections?

FWIF, I once made a gem to do that for any collection of your liking, and while working on it, I identified lists of methods where it is probably desirable to return same class: methods returning singular collection and methods returning several collections.

[Dan0042 (Daniel DeLorme)]

I think it's ok that we don't have some particular rules for Enumerables to return their own class. Rather than consistently follow a decision, I think it's good to evaluate each case on its own merits. That said, I can certainly imagine Set#select/reject/partition returning Sets. But that would have to be a separate proposal, maybe related to #16989.

Hash#reject has returned a Hash since at least 1.8. Making Hash#select return a Hash (in 1.9) has indeed been quite useful, and it would also be useful if #partition did the same. It's a bit surprising that hash.select(&b) != hash.partition(&b).first

[Dan0042 (Daniel DeLorme)]

FWIW, current usage in gems is consistent with Hash, but I did find one incompatibility in fugit-1.3.3.

actionpack-6.0.0/lib/action_dispatch/routing/mapper.rb
267:            constraints.partition do |key, requirement|

subarrays then converted with Hash[subarray]

capybara-3.29.0/lib/capybara/selector/definition/element.rb
21:    booleans, values = options.partition { |_k, v| [true, false].include? v }.map(&:to_h)

subarrays converted with .map(&:to_h)

fog-openstack-1.0.9/lib/fog/openstack/image/v1/models/images.rb
32:            custom_properties, params = headers.partition do |k, _|

subarrays converted with .map { |p| Hash[p] }

fugit-1.3.3/lib/fugit/duration.rb
71:    INFLA_KEYS, NON_INFLA_KEYS =
72:      KEYS.partition { |k, v| v[:I] }

subarrays iterated with .each do |k, a|
However there is incompatibility with keys = INFLA_KEYS.dup; keys.unshift([ :mon, { s: Fugit::Duration.parse(mon).to_sec } ])

sequel-5.24.0/lib/sequel/plugins/auto_validations.rb
167:          not_null_cols, explicit_not_null_cols = db_schema.select{|col, sch| sch[:allow_null] == false}.partition{|col, sch| sch[:default].nil?}.map{|cs| cs.map{|col, sch| col}}

subarrays converted to keys with .map{|cs| cs.map{|col, sch| col}} would have same result with hash but could be written .map(&:keys) instead

sprockets-3.7.2/lib/sprockets/cache/file_store.rb
167:          delete_caches, keep_caches = caches.partition { |filename, stat|

subarrays compatible with hash: .map(&:first) and .inject(0) { |sum, (_, stat)|

[matz (Yukihiro Matsumoto)]

We worry about compatibility. It's too hard to estimate the damage. Maybe we need a new name?

Matz.

[knu (Akinori MUSHA)]

I only wish partition had been changed this way along with select/reject...

From my experience, partition is likely used in business logic, so it is not safe to break the compatibility judging from public code search. In today's developer meeting, we discussed this and many pointed out that this functionality should have a new name, keeping partition as it is now.

[Dan0042 (Daniel DeLorme)]

knu (Akinori MUSHA) wrote in #note-6:

I only wish partition had been changed this way along with select/reject...

If most people agree "it should have been this way" then why not bite the bullet and fix to the behavior that "should have been". Better late than never.

I agree it's hard to estimate the damage. Based on the code search above my best guess is "reasonable". But isn't that what release candidates are for? In the past matz has often said "let's try it" and revert if it causes too many problems, like freezing Symbol#to_s. I think this is one of those times.

I get the feeling that a new method like partition_h would only be making this problem (mere annoyance?) worse.