-
Notifications
You must be signed in to change notification settings - Fork 20
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
WIP: allow relative paths #75
Conversation
|
||
~~~~ abnf | ||
json-path = root-selector *selector | ||
json-path = start-selector *selector | ||
start-selector = root-selector / current-item-selector |
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'm not sure whether it makes sense to start a JSONPath with a current item selector as this is not "in the middle of" an array selection, for example, so what would the current item even mean?
Also, is there a consensus among existing implementations to support such a thing?
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.
Right, the ABNF currently does not discuss nested queries, so there is no place where this could dock with.
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'm not sure whether it makes sense to start a JSONPath with a current item selector as this is not "in the middle of" an array selection, for example, so what would the current item even mean?
TL;DR: it is the item set when calling the JSONPath evaluator, it is quite useful if we are going to embed/use JSONPath with advanced tooling.
Anyway please take a look to the following comments that explains that with further details:
#59 (comment)
#59 (comment)
Also supporting @
for all JSONPaths has the good side effect of simplifying grammar when defining filter expressions (no special cases).
Also, is there a consensus among existing implementations to support such a thing?
Jayway supports that, which is an influential implementation (I think we should consider some kind of weighted consensus) and I wish to support this also on my implementation ExJSONPath.
TBH I didn't check this for all existing implementations, so further implementations supporting this syntax might exist.
@glyn: Please, may you take a look to #59 that has all the information you are looking for.
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.
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.
Actually it removes the need for any special case when using filters, since we may allow any jsonpath expression, simplifying grammar and existing implementations.
Also it has a XPath equivalent (which exists for good reasons), so we aren't introducing anything new here (I just added to this PR an example for it).
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.
@danielaparker: does replacing
The current item selector
@
when used outside a filter behaves as the document root selector unless a child node is given to the evaluator.
with
The current item selector
@
can be used to represent the current node being evaluated. The initial current node defaults to the document root unless a child node is given to the evaluator as initial current node.
looks better to you?
Maybe another phrasing might be:
The current item selector
@
can be used to represent the current node being evaluated. The initial current node defaults to the document root unless a different node is chosen.
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 think I might also replace:
When a child node is used as current item, evaluator must select it as starting
node instead of the document root when an expression starting with@
is
evaluated.
with
Therefore when a child node is set as initial current item, evaluator must select it as starting node instead of the document root when an expression starting with
@
is evaluated.
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.
@bettio wrote:
@danielaparker: does replacing
The current item selector
@
when used outside a filter behaves as the document root selector unless a child node is given to the evaluator.with
The current item selector
@
can be used to represent the current node being evaluated. The initial current node defaults to the document root unless a child node is given to the evaluator as initial current node.looks better to you?
Maybe another phrasing might be:
The current item selector
@
can be used to represent the current node being evaluated. The initial current node defaults to the document root unless a different node is chosen.
I think it's enough to say "The current item selector @
can be used to represent the current node being evaluated", full stop. That's about equivalent to the meaning of @
in JMESPath, and to the meaning of "." in XPATH 3.1, see here.
For a JSONPath evaluator, it only knows about one document, the one provided as an argument, along with the JSONPath expression. The current node is initialized to that document, and evaluation begins.
Sure, you can have higher level functions built on top of the JSONPath evaluator, that pass a different document to the evaluator depending on whether the path starts with $
or @
. Implementers can support such functions if they wish. But the evaluator doesn't know about that, it only knows about one document.
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.
For a JSONPath evaluator, it only knows about one document, the one provided as an argument, along with the JSONPath expression. The current node is initialized to that document, and evaluation begins.
I would rather stay explicit, so we make sure that it cannot be misunderstood. Therefore writing "The initial current node defaults to the document root" doesn't hurt and it helps an unambiguous understanding.
unless a child node is given to the evaluator as initial current node.
This helps remarking that @
at the beginning of an expression is not necessarily a $
synonymous according to the following proposal:
Proposal: allow paths to start with @. It operates no differently than $, but can serve as an important indicator to tooling and other systems that consume JSON Path.
my goal is making sure that starting evaluation with current item pointing to a child node and optionally referencing it with @
at the beginning of an expression is compliant with the specification.
I feel like that just stopping at "The current item selector @ can be used to represent the current node being evaluated" might leave my goal in an unclear status.
By the way when filtering will be written we'll also remark that current item is updated.
Sure, you can have higher level functions built on top of the JSONPath evaluator, that pass a different document to the evaluator depending on whether the path starts with $ or @. Implementers can support such functions if they wish. But the evaluator doesn't know about that, it only knows about one document.
For the record, my evaluator has 3 parameters: json_path_eval(document_root, current_item, path)
. It is implemented as a recursive function and document_root
is always passed unchanged, while current_item
is updated. The caller may call it doing something like:
json_path_eval(root, root.foo, "@.bar")
which is equivalent to json_path_eval(root, root, "$.foo.bar")
.
No higher level function magic here ;)
Anyway let's keep out of the discussion implementation detail, again I just need to make sure that @.bar
(with the meaning that I just showed in the example) is not left out of the spec.
Lastly the PR just aims to allow @
at the beginning of the expression, I think more PRs are needed for a complete @
formalization.
@danielaparker thanks for your feedback, looking forward to further comments :)
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.
@gregsdennis: wish to read any feedback about the wording from you too :) Your help has been valuable so far
Allow beginning paths using `@` instead of `$`.
c6423d8
to
79412af
Compare
As a summary I'm aiming to address with this PR:
I'm not addressing:
|
@bettio wrote:
Fair enough, it can be left open what the current node is initialized to, I have no objection. Daniel |
The current item selector `@` when used outside a filter behaves as the document | ||
root selector unless a child node is given to the evaluator. | ||
When a child node is used as current item, evaluator must select it as starting | ||
node instead of the document root when an expression starting with `@` is | ||
evaluated. | ||
|
||
When a child node is used as current item, the current item selector `@` selects | ||
it as starting node instead of the document root. |
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.
Usage of "node" here? I think this is one we've decided to avoid.
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 don't know who "we" is; I think we are still discussing terminology.
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'm not so sure about "child node", the term "child" is usually used for direct descendants.)
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.
Yes, we're still discussing it, which means that it's something to avoid for now.
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.
ok, I can replace node
with item
and child node
with subdocument
. I think that further improvements might be applied on future PRs.
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.
Please wait for PR#72 to settle.
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.
"document" certainly is not the terminology we want to use.
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 think that I can replace "The current item selector
" ... "with @ is evaluated.
"
with
The current item selector `@` can be used to represent the current item being
evaluated. The initial current item defaults to the document root unless a
subdocument is given to the evaluator as initial current item.
I think I can safely remove "When a child node is used as current item, evaluator must select it as starting node instead of the document root when an expression starting with @ is evaluated.
" and "When a child node is used as current item, the current item selector @ selects it as starting node instead of the document root.
" since the proposed contains the same information but with a better phrasing.
@gregsdennis Let me know if it sounds good to you so I can push changes I described here.
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 think @cabo makes a good point about resolving it in another issue when we finish discussing "node."
No worries.
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.
@gregsdennis: Ok, I'm keeping node for now, we can improve this with a future PR.
@cabo I removed document and replaced it again with "root node". I also replaced "child node" with "descendant node" which is more accurate.
So I will commit & push:
The current item selector `@` can be used to represent the current item being
evaluated. The initial current item defaults to the root node unless a
descendant node is given to the evaluator as initial current item.
Let me know if you have any further feedback, otherwise let me know you are ok for now so I can push it.
node instead of the document root when an expression starting with `@` is | ||
evaluated. | ||
|
||
When a child node is used as current item, the current item selector `@` selects |
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'm not sure who's working with English as a first language, so please don't take offense, but this could use some grammatical massaging, specifically the use of the word "the" in a few places.
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.
but this could use some grammatical massaging
It is not my first language, so any advice or feedback is welcome
Anyway I think that I'll remove this and just keep:
The current item selector `@` can be used to represent the current item being
evaluated. The initial current item defaults to the document root unless a
subdocument is given to the evaluator as initial current item.
as described in the other conversation, if you agree too that "When a child node is used as current item, the
"... is just redundant we can remove it and close this conversation since we don't need rephrasing it.
@bettio What are your plans for this PR? Thanks. |
Closing as there was no response. Please re-open if you would like to continue working on this PR. |
Draft not yet ready for merge or complete
See also GH #59