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

Adding deep versions of stringify_keys and symbolize_keys (plain and bang) for nested hashes #6060

Merged
merged 1 commit into from May 23, 2012

Conversation

@lucashungaro
Copy link
Contributor

lucashungaro commented Apr 29, 2012

I have to carry around some snippets 'cause I always use these methods (specially deep_symbolize_keys) when I use YAML files with custom app configuration. Some people I know also have their own versions, so I thought adding them to ActiveSupport was a good idea.

@josevalim
josevalim reviewed Apr 29, 2012
View changes
activesupport/lib/active_support/core_ext/hash/keys.rb Outdated
result = {}
self.each do |key, value|
if value.is_a?(Hash)
result[(key.to_s)] = value.deep_stringify_keys

This comment has been minimized.

Copy link
@josevalim

josevalim Apr 29, 2012

Contributor

Can we remove the parenthesis here in the hash access pls?

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Woops! My bad. Doing it.

@josevalim
josevalim reviewed Apr 29, 2012
View changes
activesupport/lib/active_support/core_ext/hash/keys.rb Outdated
keys.each do |key|
self[key.to_s] = delete(key)
end
values.each{ |h| h.deep_stringify_keys! if h.is_a?(Hash) }

This comment has been minimized.

Copy link
@josevalim

josevalim Apr 29, 2012

Contributor

Can we loop keys and values once?

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

I think so. I did them separately to ease the understanding of what's going on, but it would be easy to do in one step.

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Ok, after further review, I can't change the own hash inside an each block (it throws an exception). I could copy the hash and do the conversions, then change self to reference the new hash or just keep iterating over keys and values separately. What do you think?

This comment has been minimized.

Copy link
@gazay

gazay Apr 29, 2012

Contributor

Yes, I think it can be done similar to this #5939

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Using each_with_object?

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Yeah, that was what I thought at first. I'll think about a way to benchmark this and see which version is faster. Thanks!

Also, I'm repeating the logic in every method instead of reusing it (like in your example) because sometimes people will undef some of these methods. I found one case while writing this code, so I decided to just repeat the key conversion code in every method. Maybe extracting that part to a private method and reusing it would be fine.

This comment has been minimized.

Copy link
@gazay

gazay Apr 29, 2012

Contributor

By my benchmark test separate iteration is faster than my example about 1.5 times :)

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 30, 2012

Author Contributor

I've found similar results. The code that iterates over keys and then values takes ~6.4s to do deep_stringify_keys! 1 million times. The code that loops over keys and values at once copying the array and "rebuilding" self after takes ~8.9s.

So, I think I'll leave the code as it is now. Anything else I can improve upon?

This comment has been minimized.

Copy link
@gazay

gazay May 1, 2012

Contributor

I found better solution and only with one iteration:

def deep_stringify_keys!
  keys.each do |key|
    val = delete(key)
    self[key.to_s] = val.is_a?(Hash) ? val.deep_stringify_keys! : val
  end
  self
end

It faster then separate iteration

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro May 1, 2012

Author Contributor

Nice @gazay! It seems Ruby doesn't mind if you change the hash while iterating over the keys only. Good to know. :)

I made a commit using this code for deep_stringify_keys! and deep_symbolize_keys!.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 29, 2012

Related with #5587

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 29, 2012

Related with #5724

@gazay
gazay reviewed Apr 29, 2012
View changes
activesupport/lib/active_support/core_ext/hash/keys.rb Outdated
# nested hashes.
def deep_stringify_keys
result = {}
self.each do |key, value|

This comment has been minimized.

Copy link
@gazay

gazay Apr 29, 2012

Contributor

Self is unnecessary here

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Sure. It's more a style thing. Should I remove it?

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 29, 2012

Member

Yes please. We had a pull requesting from @gazay merged yesterday only changing these style things on Active Support

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Ok, no problem. :)

@gazay
gazay reviewed Apr 29, 2012
View changes
activesupport/lib/active_support/core_ext/hash/keys.rb Outdated
def deep_stringify_keys
result = {}
each do |key, value|
result[key.to_s] = value.is_a?(Hash) ? value.deep_stringify_keys : self[key]

This comment has been minimized.

Copy link
@gazay

gazay Apr 29, 2012

Contributor

Instead of calling [] on self will be better to pass just value in the false part of ternary operator

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Apr 29, 2012

Author Contributor

Yeah, way better. Missed that somehow. Thank you.

@Najaf

This comment has been minimized.

Copy link

Najaf commented May 1, 2012

Quick heads up, may or not be relevant but i18n appears to implement deep_symbolize_keys in their core_ext/hash.rb too.

Also, minor annoyance (to which I'd be willing to put together a pull-request for). It would be nice if all hashes in arrays were deep symbolized too.

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 1, 2012

Najaf, I've saw i18n's code and it only defines the method if it isn't already defined. I tried to implement it in a way that doesn't break i18n.

My initial implementation took into account hashes inside arrays but then I thought that could be a bad surprise for some people and ignored that possibility. Should we have it?

@Najaf

This comment has been minimized.

Copy link

Najaf commented May 1, 2012

+1 from me, I found it to be a bad surprise when it didn't symbolize hashes inside arrays.

@masterkain

This comment has been minimized.

Copy link
Contributor

masterkain commented May 2, 2012

+1 with backport

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 15, 2012

Hey @lucashungaro, thanks for your pull request. Perhaps you could add an entry to the changelog talking about the new methods, and some quick docs to AS Core Extensions guide before it goes in? Thanks!

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 15, 2012

Will do! :)

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 16, 2012

Ok, done. I added simple notes to the Guides mentioning the existence of the deep versions. Do you guys think this is enough or should I add "sections" dedicated to those?

Also, should I change the code to also convert keys from hashes inside arrays?

@carlosantoniodasilva
carlosantoniodasilva reviewed May 16, 2012
View changes
activesupport/CHANGELOG.md Outdated
@@ -1,5 +1,7 @@
## Rails 4.0.0 (unreleased) ##

* Adds `Hash#deep_symbolize_keys` and `Hash#deep_symbolize_keys!` to convert all keys from a +Hash+ instance into symbols *Lucas Húngaro*

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva May 16, 2012

Member

I think you can comment about both deep_symbolize_keys and deep_stringify_keys :)

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro May 16, 2012

Author Contributor

Gee, I need to rest. :P

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 16, 2012

@lucashungaro not sure it's necessary to add a specific section for them... is it, @vijaydev? Thanks.

@vijaydev

This comment has been minimized.

Copy link
Member

vijaydev commented May 16, 2012

@lucashungaro @carlosantoniodasilva No need for a specific section. I think what's in this PR is good enough. Add an example may be?

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 16, 2012

Ok, one more commit adding the changelog note and simple examples to the Guides. :)

@vijaydev

This comment has been minimized.

Copy link
Member

vijaydev commented May 16, 2012

Squash the commits into one please.

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 16, 2012

Done.

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 23, 2012

@lucashungaro I believe it's fine to merge, but somehow it does not apply cleanly anymore (probably just the changelog conflicting). Can you rebase from current master? Thanks.

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 23, 2012

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 23, 2012

Thanks! But I guess I'll have to bother you again :P

Apparently this merge c1487f6 has messed up with your pull request, and it can't be merged again :D

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 23, 2012

ooops! Sorry @lucashungaro.

@lucashungaro

This comment has been minimized.

Copy link
Contributor Author

lucashungaro commented May 23, 2012

@carlosantoniodasilva @rafaelfranca Shit happens ;). Did the rebase again.

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented May 23, 2012

@lucashungaro hehe, all the time :D.. Merging, thank you.

carlosantoniodasilva added a commit that referenced this pull request May 23, 2012
Adding deep versions of stringify_keys and symbolize_keys (plain and bang) for nested hashes
@carlosantoniodasilva carlosantoniodasilva merged commit 541429f into rails:master May 23, 2012
def deep_symbolize_keys!
keys.each do |key|
val = delete(key)
self[(key.to_sym rescue key)] = val.is_a?(Hash) ? val.deep_stringify_keys! : val

This comment has been minimized.

Copy link
@rilian

rilian Aug 30, 2012

why stringify here ?

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Aug 30, 2012

Author Contributor

Silly mistake. It's correct on master, which uses a much improved code through transform_keys :)

This comment has been minimized.

Copy link
@rilian

rilian Aug 31, 2012

huh, thanks :)

should I backport to activesupport the deep_transform_keys method that goes each Array value and transforms it for such hashes

{ :a => { :b => { :c => 3, :d => [ { :e => 4 }, { :f => 5 } ] } } }

useful for validation of hashes of model with has_many related models parsed from JSON

This comment has been minimized.

Copy link
@lucashungaro

lucashungaro Aug 31, 2012

Author Contributor

I think that would be nice. :)

@rmsy

This comment has been minimized.

Copy link

rmsy commented Oct 21, 2013

Sorry for the bump, but I just want to profess my sincere thanks for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.