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

duck-type values responding to #to_hash or #to_ary #51

Merged
merged 6 commits into from
Jan 10, 2022

Conversation

notEthan
Copy link
Contributor

@notEthan notEthan commented Jan 2, 2022

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.

@mullermp
Copy link
Contributor

mullermp commented Jan 3, 2022

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 to_a or to_h on them and pass those raw converted values into jmespath?

Copy link
Contributor

@alextwoods alextwoods left a 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.

@notEthan
Copy link
Contributor Author

notEthan commented Jan 4, 2022

What is the experience without any changes? - i.e. for your JSON data wrapped classes, are you able to call to_a or to_h on them and pass those raw converted values into jmespath?

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 #jmespath_search(expression) to the wrapper class, to help with searching documents.

lib/jmespath/nodes/function.rb Outdated Show resolved Hide resolved
lib/jmespath/nodes.rb Show resolved Hide resolved
@notEthan
Copy link
Contributor Author

notEthan commented Jan 5, 2022

I think I have the code in a good state for review, and have added a couple tests

@notEthan notEthan marked this pull request as ready for review January 5, 2022 07:57
@mullermp
Copy link
Contributor

mullermp commented Jan 5, 2022

Thanks. We'll review this shortly.

@alextwoods
Copy link
Contributor

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?

@notEthan
Copy link
Contributor Author

notEthan commented Jan 7, 2022

thanks for the additions to the tests, that was definitely needed. I added and fixed a couple more to_arys, and added a Util.as_json for recursive comparison (this made redundant the unwrap method you'd added to the tests, so I removed that). added some test cases to the compliance json to cover code changes.
also rebased against main.

Copy link
Contributor

@alextwoods alextwoods left a 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!

@notEthan
Copy link
Contributor Author

notEthan commented Jan 8, 2022

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.
looking forward to integrating support when this is merged and gets released.

@alextwoods alextwoods merged commit b9167e5 into jmespath:main Jan 10, 2022
@alextwoods
Copy link
Contributor

@notEthan - Thanks again for a great contribution. I've released this as version 1.5.0.

@brianalexander
Copy link

brianalexander commented Aug 3, 2022

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, Aws::EC2::Instance.block_device_mappings returns an array of InstanceBlockDeviceMapping that are now converted from Structs to Hashes implicitly.

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]

@mullermp
Copy link
Contributor

mullermp commented Aug 4, 2022

That seems wrong. It should be a struct. We will investigate.

@alextwoods
Copy link
Contributor

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).

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

Successfully merging this pull request may close these issues.

4 participants