-
-
Notifications
You must be signed in to change notification settings - Fork 320
Updates for Relative JSON Pointer #1400
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
Conversation
Taking the index of the result of an index manipulation was shown in the example and intended to work, but left out of the ABNF apparently by accident. Also, rework the ABNF to keep it within the text RFC line width.
Also, use <sourcecode> tag.
Somehow this was left out of the main syntax description paragraph entirely.
The current draft allows for an index adjustment of "0" which creates ambiguity because "1/foo", "1+0/foo", and "1-0/foo" are now three different ways to write a pointer with the exact same effect. This complicates round-trips between textual and functional representations, the original text would need to be preserved in order to re-constitute it correctly. There is no need for this added complication.
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.
I was asked to take a look. The ABNF looks like it matches the prose; I've left a small optimization suggestion inline.
relative-json-pointer = origin-specification json-pointer | ||
/ origin-specification "#" |
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.
relative-json-pointer = origin-specification json-pointer | |
/ origin-specification "#" | |
relative-json-pointer = origin-specification ( "#" / json-pointer ) |
It might be better to not duplicate the origin-specification
as it's not necessary; this equivalent formulation lets the syntax stay LL1-parseable.
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.
I can't seem to push to this branch. I'll create a secondary PR for this.
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.
Thank you for this work!
Replaces #1361
Adds an optional index manipulation to the syntax.It looks like this was already in the text, but the ABNF was just missing it. This PR adds the missing syntax into the ABNF.