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

Allow user to invoke query and optQuery ,with a JSONPointer #324

Merged
merged 2 commits into from
Mar 28, 2017

Conversation

dtalex
Copy link
Contributor

@dtalex dtalex commented Feb 25, 2017

It would be good for user's code readability to be able to invoke methods "query" and "optQuery" with a user-provided JSONPointer

@dtalex dtalex changed the title Allow user to invoke query and optQuery ,with a JSONPointerJson pointer on beans Allow user to invoke query and optQuery ,with a JSONPointer Feb 25, 2017
@stleary
Copy link
Owner

stleary commented Feb 27, 2017

Looks reasonable, if nothing comes up in the unit tests, and no objections from @erosb. I will add some tests. Comments need to be cleaned up. For example spelling on line 1365, and param is not a string on line 1380.

@erosb
Copy link
Contributor

erosb commented Feb 27, 2017 via email

@stleary
Copy link
Owner

stleary commented Mar 26, 2017

What problem does this code solve?
This is an enhancement, not a bug fix. API methods using JSONPointer for queries are added as a convenience.

Risks
Very low risk.

Changes to the API?
Yes, new API methods are added:

jsonObject.query(JSONPointer)
jsonObject.optQuery(JSONPointer)
jsonArray.query(JSONPointer)
jsonArray.optQuery(JSONPointer)

Will this require a new release?
No, this change can be rolled into the next release.

Should the documentation be updated?
Not needed.

Does it break the unit tests?
No.

Was any code refactored in this commit?
Yes. The query(String) and optQuery(String) methods were changed to call the new API. This was a reasonable change.
**NOTE: ** There are a few typos in the comments, which should be fixed the next time this code is touched.

Review status
Approved. The usual 3 day review period is skipped, since this pull request has already been up for a month without objection. Will merge in 1 day.

@stleary stleary merged commit 80e2ea2 into stleary:master Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants