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

Bug: Strips away key information if path is recursive and only one key/value set #39

Closed
kaelumania opened this issue Jun 11, 2015 · 10 comments
Milestone

Comments

@kaelumania
Copy link

When I use the recurse version and there is only one value set in any of the child paths, e.g.

Diplomat::Kv.put('a/b/c', 'test')
Diplomat::Kv.get('a/b', {recurse: true}, :return, :return)

The the output is just

test

and I am missing the information from which childpath/key the value came from. This behaviour is implemented at https://github.com/WeAreFarmGeek/diplomat/blob/master/lib/diplomat/rest_client.rb#L79-L81

@johnhamelink
Copy link
Member

Well spotted. The question is - should we detect if the path is recursive and return a different result or should we make it an explicit option that you set?

@johnhamelink
Copy link
Member

I've decided I'm going to return the hash, but I will do this as part of a breaking change, which according to Semver will bump the version number up to 1.0.0. I'll do this after I've merged everything else that's non-breaking in and released 0.13.0.

@johnhamelink johnhamelink added this to the 1.0.0 milestone Jul 30, 2015
@chaos95
Copy link
Contributor

chaos95 commented Dec 6, 2015

@johnhamelink According to Semver, major version 0 (that is, 0.y.z) is for initial development and does not guarantee any protection against breaking changes. You should feel free to break things between minor revisions when you're still in pre-release.

@johnhamelink
Copy link
Member

@chaos95 Agreed, but considering the amount of people using this gem, we are now in release whether we like it or not 😄

@aaronbbrown
Copy link
Contributor

Would be super sweet to see this get fixed, as I'm running into the same problem.

@EugenMayer
Copy link
Contributor

Iam bumping this issue, since with 1.1.0 we have now convert_to_hash: true and this we can implement a good behavior here.

Right now, if you ask for test/this/out/key = 'test' with

Diplomat::Kv.get('test/', {convert_to_hash: true, recurse: true}, :return)

You get a plain parse exception that test cannot be "eached"

undefined method `each' for "test":String

@johnhamelink it seemed to be on the 1.0.0 roadmap but either it got forgotten or this issue is not the right home for my described bug. Could you tell me whether to create a new context for this?

For more cleanup to this ssue:
Pull request #51 should be closed since it is not based on the current hash-parser or was not aware back then in 2015, that it will be existing. I think, the patch for the issue above is far more trivial

@snoremac
Copy link

If it helps anyone, our fork changes behaviour such that single-valued results are packaged the same way as multi-valued ones.

https://github.com/caradvice/diplomat

@EugenMayer
Copy link
Contributor

@snoremac wondering why you did not create a pull request? Thank you for your work!

@pklingem
Copy link

@snoremac thoughts on PRing?

@pierresouchay
Copy link
Member

This has been added in 2.1.0

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

No branches or pull requests

8 participants