-
Notifications
You must be signed in to change notification settings - Fork 20
Editorial improvements #41
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
Thanks for feedback from Daniel Parker and Carsten Bormann.
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.
Quick look...
draft-normington-jsonpath-latest.md
Outdated
array-index = integer | ||
|
||
integer = [%x2D] (%x30 / (%x31-39 *%x30-39)) | ||
integer = ["-"] ("0" / (%x31-39 *%x30-39)) |
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.
You may want to make use of DIGIT and DIGIT1 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.
Whoops. Of course.
draft-normington-jsonpath-latest.md
Outdated
|
||
An array slice is a union element consisting of two or three integers (in | ||
base 10 and which may be omitted) separated by colons. | ||
An array slice is a union element consisting of optional integers (in base 10) separated by colons. |
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 we need to be more aware about saying "is" when we mean "is represented by".
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, I mean "is" rather than "is represented by". But the phrase "is a union element consisting of" is causing a lot of grief. It probably is better to make the array slice description independent of where it is used, thus:
An array slice is a union element consisting of optional integers (in base 10) separated by colons. | |
An array slice consists of optional integers (in base 10) separated by colons. |
draft-normington-jsonpath-latest.md
Outdated
array-index = integer | ||
|
||
integer = [%x2D] (%x30 / (%x31-39 *%x30-39)) | ||
integer = ["-"] ("0" / (%x31-39 *%x30-39)) |
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.
What was the guidance around using quoted characters vs their hex equivalent?
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.
That change was based on input from @cabo.
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.
Could you link to the input? Why have both?
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.
array-slice = [ start ] ws ":" ws [ end ] | ||
[ ws ":" ws [ step ] ] | ||
start = integer | ||
end = integer | ||
step = integer |
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.
Does the markdown translation align the =
?
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.
No. Does that matter?
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.
Typically I see them aligned for readability, but technically, no.
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 suggest we leave that for now and do a fine-tuning editorial pass later for consistency.
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.
Couple questions and a suggestion
380417d
to
c88f2fa
Compare
Use term "JSON value" instead of "JSON document". Use "JSONPath" instead of "JSON Path" for consistency with Gössner.
c88f2fa
to
658ed7a
Compare
I'd like to merge so that @cabo can proceed to raise PRs which add introductory material from the Gössner draft. None of this is set in concrete, so there will be ample opportunities to revisit the text. |
@gregsdennis Merged. Let me know if you have outstanding issues and we can address them separately. |
Thanks for feedback from Daniel Parker and Carsten Bormann.