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

Ensure `HashWithIndifferentAccess#transform_keys` to return `HashWithIndifferentAccess` #30728

Merged
merged 1 commit into from Sep 27, 2017

Conversation

Projects
None yet
5 participants
@y-yagi
Member

y-yagi commented Sep 27, 2017

Currently, #transform_values, #select and #reject return instance of HashWithIndifferentAccess. But #transform_keys returns instance of Hash. This behavior is a bit confusing.

I think that HashWithIndifferentAccess#transform_keys should also return instance of HashWithIndifferentAccess as well as other methods.

Ensure `HashWithIndifferentAccess#transform_keys` to return `HashWith…
…IndifferentAccess`

Currently, `#transform_values`, `#select` and `#reject` return instance
of `HashWithIndifferentAccess`. But `#transform_keys` returns instance
of Hash. This behavior is a bit confusing.

I think that `HashWithIndifferentAccess#transform_keys` should also return
instance of `HashWithIndifferentAccess` as well as other methods.
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Sep 27, 2017

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Sep 27, 2017

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Sep 27, 2017

Member

Looks like missing test case for transform_values.
Could you also add a test case for that?

Member

kamipo commented Sep 27, 2017

Looks like missing test case for transform_values.
Could you also add a test case for that?

@y-yagi

This comment has been minimized.

Show comment
Hide comment
@y-yagi

y-yagi Sep 27, 2017

Member

The test of transform_values is in transform_values_test.rb.
https://github.com/rails/rails/blob/master/activesupport/test/core_ext/hash/transform_values_test.rb#L24..L30
However, I think that it was easier to understand if it be in hash_with_indifferent_access_test.rb. Should we move it?

Member

y-yagi commented Sep 27, 2017

The test of transform_values is in transform_values_test.rb.
https://github.com/rails/rails/blob/master/activesupport/test/core_ext/hash/transform_values_test.rb#L24..L30
However, I think that it was easier to understand if it be in hash_with_indifferent_access_test.rb. Should we move it?

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Sep 27, 2017

Member

Ah, I see. It is okey as is for now.

Member

kamipo commented Sep 27, 2017

Ah, I see. It is okey as is for now.

@kamipo kamipo merged commit 0d825a6 into rails:master Sep 27, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@y-yagi y-yagi deleted the y-yagi:ensure_transform_keys_of_hwida_to_return_hwida branch Sep 27, 2017

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Sep 27, 2017

Member

I'm a bit concerned about how changing this might impact users, given that HWIA does an extra key-transform for itself.

Is it worth a deprecation?

Member

matthewd commented Sep 27, 2017

I'm a bit concerned about how changing this might impact users, given that HWIA does an extra key-transform for itself.

Is it worth a deprecation?

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Sep 27, 2017

Member

I feel that transform_keys and transform_keys! should behave consistently.
Originally transform_keys! returns itself and it is an instance of HashWithIndifferentAccess.
But previously transform_keys returned an instance of Hash.
Since bang methods returns an instance of HashWithIndifferentAccess, I think that non bang methods should also keep the class.

WDYT?

Member

kamipo commented Sep 27, 2017

I feel that transform_keys and transform_keys! should behave consistently.
Originally transform_keys! returns itself and it is an instance of HashWithIndifferentAccess.
But previously transform_keys returned an instance of Hash.
Since bang methods returns an instance of HashWithIndifferentAccess, I think that non bang methods should also keep the class.

WDYT?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Sep 27, 2017

Member

I agree we should make the change.. I was just wondering whether it's likely to break existing code. But as it only affects symbols, it's probably fine. 🤷🏻‍♂️

Member

matthewd commented Sep 27, 2017

I agree we should make the change.. I was just wondering whether it's likely to break existing code. But as it only affects symbols, it's probably fine. 🤷🏻‍♂️

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