Skip to content

Add dig function #573

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

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Add dig function #573

merged 1 commit into from
Feb 16, 2016

Conversation

mks-m
Copy link
Contributor

@mks-m mks-m commented Feb 9, 2016

Makes deep lookup in nested hashes(and/or arrays) easier.

Usage:

dig($hash, ["key1", "key2", ...])
dig($hash, ["key1", "key2", ...], "default")
dig(
  [{"a" => {"b" => "c"}}],
  [0, "a", "b"]) # => "c"

Returns nil or optional default arg if at any point in the chain key couldn't be looked up.

@elyscape
Copy link
Contributor

elyscape commented Feb 9, 2016

This already exists as try_get_value.

@elyscape
Copy link
Contributor

elyscape commented Feb 9, 2016

That being said, it might make sense to extend try_get_value to support taking an array as its second argument so that no string splitting needs to be performed.

@mks-m
Copy link
Contributor Author

mks-m commented Feb 9, 2016

with #dig making it's way into standard ruby (http://ruby-doc.org/core-2.3.0/Hash.html#method-i-dig) i think it makes sense to preserve the name. i also like implementation and api more, but i'm subjective here obviously.

@mks-m
Copy link
Contributor Author

mks-m commented Feb 9, 2016

how would you feel about me reimplementing #try_get_value in terms of #dig function, and marking it deprecated?

@elyscape
Copy link
Contributor

elyscape commented Feb 9, 2016

Might be a good idea. Pinging @DavidS and @hunner for input.

@mks-m
Copy link
Contributor Author

mks-m commented Feb 11, 2016

i've dropped some test cases from try_get_value but it remains soft-backwards-compatible, meaning it will produce an error instead of unexpected value in some cases. i don't think it's good idea to tolerate lookup functions being called on values that are not indexable.

'b2',
'b3' ]}}

$value = try_get_value($data, ['a', 'b', '2'], 'not_found')
Copy link
Contributor

Choose a reason for hiding this comment

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

should be dig


it 'should be able to return a single value' do
is_expected.to run.with_params('test').and_return('test')
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should actually stay, to check for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the purpose of this function is to do a lookup on nested data structure. passing string where nested data structure is expected is just wrong and dig() will fail explicitly in this case.

@tphoney
Copy link
Contributor

tphoney commented Feb 11, 2016

Thanks for the work. Could you squash the commits as well.

@mks-m
Copy link
Contributor Author

mks-m commented Feb 11, 2016

@tphoney do you mean squash and push --force into same branch?

@HelenCampbell
Copy link
Contributor

@keymone Yep! I usually do something like: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html , then push to the same branch to update this PR with your changes. Also, would you be able to add some info in the readme for your new function? Thanks!

@mks-m
Copy link
Contributor Author

mks-m commented Feb 15, 2016

@HelenCampbell @tphoney done

@tphoney
Copy link
Contributor

tphoney commented Feb 15, 2016

The test cases that @hunner marked to be kept are important. The try_get_value function should still be able to function with a single parameter, otherwise it might affect how current users are using try_get_value.

@HelenCampbell
Copy link
Contributor

OR another possible solution to that would be to keep the try_get_value function alone, and add dig as an additional function. The worry is that the implementation in this PR may break some existing setups.

@mks-m
Copy link
Contributor Author

mks-m commented Feb 15, 2016

@tphoney try_get_value promotes bad coding practices by being tolerant to bad input data. if somebody's codebase depends on bad coding practices, trying to be backward compatible with that will only make everything worse.

@HelenCampbell i could revert my changes to try_get_value, but breeding duplicate functions with subtly different functionality is a recipe for disaster. i'd love to see stdlib as clean and strict as possible.

Deprecates #try_get_value()
@mks-m
Copy link
Contributor Author

mks-m commented Feb 16, 2016

@hunner i've reverted spec changes and preserved original try_get_value functionality

cc/ @tphoney @HelenCampbell

@hunner
Copy link
Contributor

hunner commented Feb 16, 2016

@keymone Thanks! I agree that bad coding practices should not be promoted, but hopefully the deprecation will get people to migrate to dig and correct their practices.

hunner added a commit that referenced this pull request Feb 16, 2016
@hunner hunner merged commit e2206fd into puppetlabs:master Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants