-
Notifications
You must be signed in to change notification settings - Fork 37
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
duck-type values responding to #to_hash or #to_ary #51
Conversation
Thanks for opening up a draft PR. I think what you're trying to do makes sense. What is the experience without any changes? - i.e. for your JSON data wrapped classes, are you able to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is a reasonable change, but see comment re: Struct
.
I can pass the JSON that my classes wrap to jmespath, yes, but then lose the functionality that is added by the wrapper I have implemented. my goal is to search the wrapped document and get wrapped children out as the search result(s). the wrapping I work with is associating JSON data with JSON schemas (per the specification at https://json-schema.org/) describing those data. so if I search within a document described by a schema, the resulting children get returned associated with subschemas that describe them. the library implementing this is here, in case that is of any interest: https://github.com/notEthan/jsi if this is merged and released I'll add a method something like |
I think I have the code in a good state for review, and have added a couple tests |
Thanks. We'll review this shortly. |
I've added CI back to this project and merged those changes in. I've also ported the compliance tests into the implicit conversion test cases - there were a few failures in functions which I've fixed. There are a few other failures in the compliance tests w/ implicit conversion that I'm not positive how to address (they may be issues in my spec code as well) - could you take a look at those cases? |
thanks for the additions to the tests, that was definitely needed. I added and fixed a couple more to_arys, and added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great contribution and thanks for the extra testing!
glad to hear. thanks very much for your reviews and additions. I am quite happy to find active maintainers here, the project looked very quiet. also, aside, this is among the best-written libraries I've worked with or contributed to. |
@notEthan - Thanks again for a great contribution. I've released this as version 1.5.0. |
For people searching google: Any developers using the AWS SDK for Ruby v2 that has a dependency on jmespath updating to 1.5.0 or greater, To correct errors, update dot operator to bracket notation for value access. Old way: block_devices = Aws::EC2::Instance.new(instance_id, client: EC2_CLIENT_OBJECT).block_device_mappings
data_block_device = block_devices.find { |bd| bd.device_name == "/dev/xvdb" }
data_block_device.ebs.volume_id New way: block_devices = Aws::EC2::Instance.new(instance_id, client: EC2_CLIENT_OBJECT).block_device_mappings
data_block_device = block_devices.find { |bd| bd[:device_name] == "/dev/xvdb" }
data_block_device[:ebs][:volume_id] |
That seems wrong. It should be a struct. We will investigate. |
Thanks for reporting that - I agree with @mullermp - it should be a struct there. I have confirmed this does NOT impact V3 of the SDK (note: V2 of the SDK is no longer supported). |
I work with JSON data wrapped in classes which behave like Hash and Array, which respond to #to_hash / #to_ary to support implicit conversion (as described in https://docs.ruby-lang.org/en/master/doc/implicit_conversion_rdoc.html )
this PR lets jmespath be used with these implicitly convertible objects. it is in draft because I have updated no tests or documentation, and may be incomplete in other ways. if an active maintainer can tell me it might be merged, I will bring it to completion.