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

Implement Hash#path_to_key #1847

Closed
wants to merge 6 commits into from

Conversation

rudy-on-rails
Copy link

@rudy-on-rails rudy-on-rails commented Mar 21, 2018

This PR is a first attempt to contribute to MRI, done at the RubyHackChallenge at Cookpad office in Bristol. I apologize in advance for anything that might be missing here, in relation to how this should be approached.

It has been started with ko1/rubyhackchallenge#41. After the implementation, me and @skateman had a chart with @matz about how applicable would it be to have that code merged.

Matz suggested that a method like this, described afterwards in ko1/rubyhackchallenge#44, would have more applicability and just generally be more useful.

@rudy-on-rails
Copy link
Author

@skateman This is the implementation I came up with.

@nobu
Copy link
Member

nobu commented Mar 21, 2018

Where h = {a: 1}, both of h.path_to_key(:a) and h.path_to_key(:b) return [].
Is this intentional?

For recursive hash, h = {}; h[:x] = h, h.path_to_key(:a) returns [:x], but h doesn't contain :a.

@rudy-on-rails
Copy link
Author

@nobu Interesting comment....
Number 1 is intentional, yes, although might not be the most ideal way of exposing that the path is unexistent or it's in the first-level (root) of the Hash object.

About number two, I will add a test for this case and fix it.

hash.c Outdated
keys = rb_hash_keys(hash);
prev_hsh = hash;

for (int i = 0; i < RARRAY_LEN(keys); ++i)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declaration in for is not C89.

@nobu
Copy link
Member

nobu commented Mar 24, 2018

Why not nil when the key does not exist?

@rudy-on-rails
Copy link
Author

I think it might make more sense, @nobu . I will check it out on how can I approach that and change it.

@nobu
Copy link
Member

nobu commented Mar 24, 2018

I guess that you'd expect {a: {b: 0, x: 1}}.path_to_key(:x) to be [:a].

@rudy-on-rails
Copy link
Author

Yes @nobu. Just fixed that and another recursive hash issue...

@rudy-on-rails
Copy link
Author

rudy-on-rails commented Mar 24, 2018

I will have to slightly change the implementation of this, because hashes like

hsh = {:x=>{:e=>1}, :y=>[1, 2, 3], :z=>{:a=>{:w=>100}}}
hsh.path_to_key(:w)

will be expected to return [:z, :a] but are not in the moment due to the way sub-hashes are analised.

@nobu
Copy link
Member

nobu commented Mar 24, 2018

{a: {}, b: {x: 1}}.path_to_key(:x) returns nil.

@nobu
Copy link
Member

nobu commented Mar 24, 2018

h = {a: {}}; h[:a][:b] = h; h.path_to_key(:x) doesn't return.

@nobu
Copy link
Member

nobu commented Mar 24, 2018

What do you expect when a hash has multiple paths to the same key, e.g., {a: {x: 0}, b: {x: 1}}.path_to_key(:x)?

@rudy-on-rails
Copy link
Author

rudy-on-rails commented Mar 24, 2018

The right expectation would be [:a], the first path found that matches.

@rudy-on-rails
Copy link
Author

Hey @nobu. Can you re-check this? I have re-implemented using a recursive approach, but I don't know how much my recursion design might hurt some other parts. I've used some of the recursions over the hash as base. Cheers

@hsbt
Copy link
Member

hsbt commented Mar 27, 2018

@rudy-on-rails Can you address this feature request to https://bugs.ruby-lang.org?

We do not handle new feature on GitHub.

@rudy-on-rails
Copy link
Author

Hey @hsbt , sure thing. I will write the rationale there.

@rudy-on-rails
Copy link
Author

@k0kubun k0kubun changed the base branch from trunk to master August 15, 2019 17:34
@k0kubun
Copy link
Member

k0kubun commented Aug 17, 2019

It seems to have a conflict now. Could you rebase this from master?

@k0kubun
Copy link
Member

k0kubun commented Aug 19, 2019

Let me close this as it has not been updated for a while. Please reopen this after resolving conflicts. Thanks.

@k0kubun k0kubun closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants