-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat(jmespath): add tree interpreter #2265
Conversation
I feel like tests are require here. |
Agree, and I have implemented them. I have around 900 unit tests with 100% coverage that you can see in the Initially the whole implementation of this utility was in a single PR but I decided to break it down into smaller PRs to make the review easier. Breaking down the implementation into separate PRs was fairly easy but doing the same for the tests will take many hours. My plan was to get this plus 2 subsequent PRs for the remainder of the implementation and then open a PR dedicated to adding tests only (again, their implementation is there already). If I merged the tests now we'd have CI red because some of the tests would fail due to missing modules that will be added in the next 2 PRs. Also, the JMESPath module is currently marked as private, so even if we made a release right now it would not get published. Do you think this would work? Otherwise I can put the remainder of the implementation plus the tests in this PR, but that'll result in over a thousand LOC (primarily due to the test spec being verbose). Let me know how you'd like to proceed @sthulb. |
The tests that will cover this and all other JMESPath PRs are in this PR: #2271 |
Quality Gate passedIssues Measures |
Description of your changes
This PR adds a
TreeInterpreter
class to the JMESPath utility.The tree interpreter serves as a component responsible for evaluating the Abstract Syntax Tree (AST) generated during the parsing process against a given JSON document.
The purpose of the tree interpreter is to interpret the meaning of the JMESPath expression represented by the AST and to extract the relevant data from the JSON document accordingly.
Related issues, RFCs
Issue number: #2209
Checklist
Breaking change checklist
Is it a breaking change?: NO
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.