Skip to content

Conversation

@alerque
Copy link
Contributor

@alerque alerque commented Feb 16, 2024

The ABNF has recieved a major overhaul with difrenet formatting, and also along the way several small changes and fixes landed. These changes are not reflected in the relevant snippets in the syntax document.

This brings all the snippets up to date with the actual ABNF. One could easily make a case that the formatting doesn't need to match and each snippet could have its own whitespace choices to match the local context, but it was much easier to find changes and reason about them if the lines exactly matched the ABNF file, so I went ahead and updated all the formatting.

In the process the actual changes showed through. Some changes are typo fixes (e.g. reserve-escapereserved-escape), others have more exact definitions (e.g. reserved-keyword and reserved-statement), and a couple items have just been moved around in the source. I moved a couple definitions when they were not relevant to the bit of the document to closer match where they are in the ABNF (e.g.g quoted-char and reserved-char).

Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

While the actual rules should absolutely be the same, I don't think the whitespace changes make sense.

@alerque alerque requested a review from eemeli February 16, 2024 09:56
@aphillips aphillips added syntax Issues related with syntax or ABNF fast-track Editorial change permitted to use fast-track merge rules editorial Issue is non-normative specification Issue affects the specification LDML45 labels Feb 16, 2024
Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

The changes look good to me. I'll admit that I didn't actually check them against the ABNF.

Copy link
Collaborator

@stasm stasm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @alerque! I looked at syntax.md from your branch and message.abnf from main side by side, and I think they're equivalent, modulo the ordering of productions.

Comment on lines +292 to +293
quoted-char = content-char / s / "." / "@" / "{" / "}"
reserved-char = content-char / "."
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are already listed elsewhere in syntax.md. Is there a reason to bring them here, rather than keeping them in their current places?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing this? Where are the productions doubled in @alerque's text?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The quoted-char is currently under Literals right after quoted, and reserved-char under Reserved Annotations right after reserved-body. The proposed change moves their listings from those places (which are their only users) to here.

As is, their meaning is obvious from context. If they're moved here, then we should add some text here explaining what they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those definitions moved in the abnf source too, and I found their new/current locations much easier to grok than the old ones the syntax explanations were using because they are now grouped with more related context with other similar definitions.

If consensuses is that isn't a good change they can go back to their original places, but the abnf snippets will no longer be all blocks where the whole snippet comes from a contiguous bit of source. Having all the snippets match some contiguous bit from the original file makes it quite a bit easier to verify that it is in sync.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As we do not intend to make further syntax changes, mirroring such changes in syntax.md as well as the ANBF should not need to be done later.

I have a weak preference for keeping these as they currently are, but I'm fine to go with whatever most people think.

@aphillips
Copy link
Member

I'm going to merge this. We can tweak the duplicates in the spec tomorrow, if we feel strongly enough about it.

@aphillips aphillips merged commit aa780d2 into unicode-org:main Feb 16, 2024
@alerque alerque deleted the update-abnf-snippets-in-spec branch February 17, 2024 19:50
@eemeli eemeli added this to the LDML 45 milestone Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editorial Issue is non-normative fast-track Editorial change permitted to use fast-track merge rules specification Issue affects the specification syntax Issues related with syntax or ABNF

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants