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

Add an appendix with collected ABNF grammar #498

Merged
merged 6 commits into from
Aug 8, 2023
Merged

Conversation

cabo
Copy link
Member

@cabo cabo commented Aug 8, 2023

We don't have to merge this, but this PR shows how to do it.

@cabo cabo requested a review from glyn August 8, 2023 07:26
@timbray
Copy link
Contributor

timbray commented Aug 8, 2023 via email

Copy link
Collaborator

@glyn glyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest capitalising Normalized Path as usual.

Also I wanted to check that scripts/gen.sh no longer working is to be expected.

draft-ietf-jsonpath-base.md Outdated Show resolved Hide resolved
draft-ietf-jsonpath-base.md Outdated Show resolved Hide resolved
<!-- Update the collected grammar files using `make sourcecode`, which -->
<!-- is currently manual as it creates a little circular dependency. -->
<!-- The filenames of the ::includes are likely to change when -->
<!-- kramdown-rfc-extract-sourcecode handles filenames better. -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this branch, my workflow breaks. I normally issue scripts/gen.sh, but this now fails with:

 Created file draft-ietf-jsonpath-base.txt
 Created file draft-ietf-jsonpath-base.html
stdin(212:0): error: Rule jsonpath-query was already defined on line 0 of stdin
stdin(213:0): error: Rule segments was already defined on line 1 of stdin
stdin(214:0): error: Rule root-identifier was already defined on line 4 of stdin
...
stdin(389:0): error: Rule normal-index-selector was already defined on line 207 of stdin
parsing failed: 78 errors encountered

make however succeeds (after I installed npm), so maybe this doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with duplicating the ABNF is that ABNF doesn't accept duplicated rules (we learned from that in CDDL, which happily accepts [and checks!] duplicated rules).
We could change gen.sh to use sourcecode/abnf as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with duplicating the ABNF is that ABNF doesn't accept duplicated rules (we learned from that in CDDL, which happily accepts [and checks!] duplicated rules).

Fair enough.

We could change gen.sh to use sourcecode/abnf as well.

Yes please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with duplicating the ABNF is that ABNF doesn't accept duplicated rules (we learned from that in CDDL, which happily accepts [and checks!] duplicated rules).

Fair enough.

We could change gen.sh to use sourcecode/abnf as well.

Yes please.

Done.

Now we just have to remember to do a make sourcecode manually; you could add this to gen.sh but because of the circular dependency I'm refraining from that now. (We also could put the bap checks in make sourcecode... But then we require people to have bap installed.)

cabo and others added 2 commits August 8, 2023 11:08
Co-authored-by: Glyn Normington <glyn.normington@gmail.com>
Co-authored-by: Glyn Normington <glyn.normington@gmail.com>
@cabo
Copy link
Member Author

cabo commented Aug 8, 2023

So, what should the RFC Editor do?

Well, I hope they don't have to do anything, as the ABNF is already fixed.
But if they do, it would be good if they did the AUTH48 in markdown, as recently has been done a number of times as an experiment.
Of course, we don't have a right to be part of such an experiment, but then RPC knows us well.

@@ -13,4 +13,5 @@ endif


sourcecode: draft-ietf-jsonpath-base.xml
rm -rf sourcecode.ba?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this not remove sourcecode.bak?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this not remove sourcecode.bak?

It does. kramdown-rfc-extract-sourcecode then creates a new backup of the existing sourcecode directory.
A bit belt and suspenders, but I wanted to make sure the tool doesn't delete user information.
(We could remove the existing sourcecode directory as well, but then recovery is a bit harder if the sourcecode extraction fails for some reason.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't express myself very well. Why doesn't the code say the following?

Suggested change
rm -rf sourcecode.ba?
rm -rf sourcecode.bak

Are there some other files/directories matching sourcecode.ba? that also need removing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if the backup mechanism can't create a .bak, it will create .bal, .bam etc.
So if you did an extract-sourcecode manually a few times, you might have all these dropping.
.ba? takes care of all ~15 of them.

@glyn
Copy link
Collaborator

glyn commented Aug 8, 2023

We don't have to merge this, but this PR shows how to do it.

BTW I really like this PR and think we should merge it. It's a convenience to certain users of the spec at very little cost to us or readers who are not interested in the collected ABNF.

Copy link
Collaborator

@glyn glyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good thanks. The ba?->bak change isn't a big issue.

@cabo cabo merged commit 87367bb into main Aug 8, 2023
2 checks passed
@cabo
Copy link
Member Author

cabo commented Aug 8, 2023

(Based on an ARTART review comment by Darrel Miller.)

@cabo cabo deleted the collected-grammars branch August 9, 2023 05:10
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.

None yet

3 participants