Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Hash#transform_keys behavior to always return a Hash instance #24517

Merged
merged 1 commit into from
Apr 15, 2016

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Apr 12, 2016

Summary

A failure in mongoid's test suite alerted me to a behavior difference between activesupport 4.1 and 4.2:

  • activesupport 4.1: Hash#symbolize_keys returns a Hash.
  • activesupport 4.2: Hash#symbolize_keys returns an instance of self.class.

Here is the related commit.
This behavior change is significant when self inherits from Hash. For example, a BSON::Document (defined in the bson gem, on which mongoid depends) inherits from Hash:

> ActiveSupport.version
=> Gem::Version.new("4.1.15")
> doc = BSON::Document.new("a" => 1)
=> {"a"=>1}
> doc.symbolize_keys
=> {:a=>1}
> doc.symbolize_keys.class
=> Hash
> ActiveSupport.version
=> Gem::Version.new("4.2.0")
> doc = BSON::Document.new("a" => 1)
=> {"a"=>1}
> doc.symbolize_keys
=> {"a"=>1}
> doc.symbolize_keys.class
=> BSON::Document

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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.

Please see the contribution instructions for more information.

@arthurnn
Copy link
Member

I wonder if returning the actual class type makes more sense indeed.
If we change this to now return a Hash , people that uses HWIA will now get a Hash back when calling that method, and I don't know if thats expected.
That said, if we wanna keep returning the class.new instance, we need to change the comment which says: Returns a new hash with all keys converted using the +block+ operation..

A solution for Mongoid, would be to stop extending Hash object. Thats a rule of thumb I learned in ruby, and try to avoid as much as possible. cc @tenderlove which loves extending core ruby classes .

@estolfo
Copy link
Contributor Author

estolfo commented Apr 12, 2016

Although I submitted this change to always return a Hash from the transform_keys method, I do think returning an instance of self.class makes more sense. Returning a Hash, regardless of what self is, seems unexpected to me. That said, this pull request was intended to restore previous behavior and was originally opened against 4.2-stable.
If the current behavior (returning an instance of self.class) will stay in 5.0, it should be documented correctly and be marked as a behavior change. Let me know what you decide and how I can help by updating the pull request. Thanks!

@rafaelfranca
Copy link
Member

This is how select and reject works on Hash subclasses. They return Hash not the actual class, so we should match that behavior.

@sgrif
Copy link
Contributor

sgrif commented Apr 12, 2016

Agreed, we should match Ruby's behavior on other relevant methods. Subclasses can override this if need be (or not subclass Hash!)

@estolfo
Copy link
Contributor Author

estolfo commented Apr 15, 2016

Thanks for your comments. I agree that matching Ruby's behavior is best.
Has a consensus been reached and can this be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants