Change Hash#transform_values to return a Hash instance for subclasses of Hash

[eitoball]

Summary

In #24517, Hash#transform_keys' returns aHashinstance. Then, I think thatHash#transform_values` should do the same.

Additionally, when it is empty, Hash#transform_keys' already returns{}`.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[matthewd]

While I agree transform_keys and transform_values should behave the same, I think #24517 was incorrect, and we should revert it instead. (@rafaelfranca and I discussed this recently, but I hadn't yet followed up on it.)

Specifically, I think Hash#select is a much weaker precedent than Hash#merge, which retains the subclass.

select feels more like "give me a hash, choosing these elements from the existing one", whereas merge (and transform_*) feels more... well, transformative: "give me a variant of this hash, changed in the following way".

3e408a2 will need some attention. Is it fast because it's quicker to check empty? than it is to call each (seems unlikely), or is it fast because {} is faster than self.class.new? 😕 (cc @sgrif)

[sgrif]

.empty? is definitely faster than .each. I agree that it should behave the same in both cases. I'm in favor of returning a hash subclass, as Ruby has pretty explicitly moved towards that for all methods which return hashes. I don't feel strongly about it however.

[rafaelfranca]

BTW, ruby 2.4 introduced methods to do this, we should match the behavior.

[eitoball]

BTW, ruby 2.4 introduced methods to do this, we should match the behavior.

Tried in ruby 2.4.0 preview 2. It returns a Hash.

$ ruby -ve '
quote> class HashDescendant < Hash
quote>   def initialize(elements = nil)
quote>     super(elements)
quote>     (elements || {}).each_pair { |key, value| self[key] = value }
quote>   end
quote> end
quote> mapped = HashDescendant.new(a: "a", b: "b").transform_values { |v| v.to_sym }
quote> p mapped.class
quote> '
ruby 2.4.0preview2 (2016-09-09 trunk 56129) [x86_64-darwin15]
Hash

According https://github.com/ruby/ruby/blob/trunk/hash.c#L1801-L1829 , it calls rb_hash_new.

[kamipo]

Closing this since active_support/core_ext/hash/transform_values.rb has been deprecated by #32034.