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

Propose reserving additional sigils for future use #374

Merged
merged 13 commits into from
Apr 24, 2023
Merged

Conversation

aphillips
Copy link
Member

@aphillips aphillips commented Apr 10, 2023

This proposal proposes to reserve sigils for future use with reserved sigils allowed in the same place that function names are currently, e.g. standalone ({%foo}), with options ({%foo key=val}), or subsidiary to variable/literals ({|quote me| %foo} or {$var %foo}).

A case could be made for reserving a comment syntax that allows a placeholder with prose in it (and I would suggest using the % for it). @eemeli's proposal is effectively this. I didn't propose this because of the impact on parsers in the future. Let's discuss.

Following our decisions in the 2023-04-10 call, updating to make the reserved sigils introduce expressions containing undifferentiated text. Note that I reused the text construct, which requires { and } to be escaped (for the benefit of parsers). I also corrected the list of reserved sigils. I left out a few potential sigils, specifically U+0060, (, ), _, and =.

This proposal proposes to reserve sigils for future use with reserved sigils allowed in the same place that function names are currently, e.g. standalone (`{%foo}`), with options (`{%foo key=val}`), or subsidiary to variable/literals (`{|quote me| %foo}` or `{$var %foo}`).

A case could be made for reserving a comment syntax that allows a placeholder with prose in it (and I would suggest using the `%` for it). @eemeli's proposal is effectively this. I didn't propose this because of the impact on parsers in the future. Let's discuss.
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.

@aphillips Did I understand right from the call that you'd be updating this PR?

@aphillips
Copy link
Member Author

@eemeli That's correct.

@aphillips aphillips requested a review from eemeli April 15, 2023 08:25
@aphillips
Copy link
Member Author

Note: this PR is still missing the work on the spec prose.

spec/message.abnf Outdated Show resolved Hide resolved
aphillips and others added 3 commits April 15, 2023 12:25
I think this works. I'm going to commit it to the PR and then do some additional cleanup.

Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
- align some items
- move `function` with `variable`
- move `reserve-escape` with the other escapes
@aphillips aphillips requested a review from eemeli April 15, 2023 10:32
Note that this change also removed the undefined term "operand" in favor of just using literal and variable (which are defined terms). It also imposes 2119 keywords.
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.

Looks good! I think we'll want to add a section on reserved to the syntax text, but I like the general shape of this.

spec/syntax.md Outdated Show resolved Hide resolved
spec/message.abnf Outdated Show resolved Hide resolved
@aphillips aphillips requested a review from eemeli April 16, 2023 10:28
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.

A few requested changes documented inline. Furthermore, the error handling & fallback string representations in formatting.md should be updated as a part of this change.

spec/syntax.md Outdated
@@ -108,7 +109,7 @@ let hello = new MessageFormat('{Hello, world!}')
hello.format()
```

### Expressions
### Placeholder Expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed at our last meeting, an expression is also used for variable definitions and for selectors; not just placeholders. I think changing that terminology should be beyond the scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the links work in the spec doc by expanding the term here. Note that the term "placeholder" doesn't appear in the ABNF and is not currently defined anywhere (else). The syntax of an expression includes the {/} markers and so I concluded that it was reasonable to say that all expressions are placeholders. The text recognizes that placeholder expressions can appear in variable definitions and selectors.

The easier way to fix this would be to use singular instead of plural and I'll do that for now.

spec/syntax.md Outdated
@@ -345,20 +346,17 @@ Whitespace within a _pattern_ is meaningful and MUST be preserved.

### Expressions

**_Expressions_** can either start with an operand or a function call.
_Expressions_ ***must*** start with a _literal_, a _variable_, or an _annotation_, or consist of a _reserved_ string. An _expression_ ***must not*** be empty.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thus far the spec markdown files have used Semantic Line Breaks, which make it easier to track changes within paragraphs. Would it be possible to continue doing so? These line breaks have no effect on the rendered spec.

Also, this sentence is a little bit wrong now that reserved may follow an argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will touch up the formatting.

I missed correcting this when making the change. I mean, technically, it's correct, but the reserved sequence is just part of an annotation :-).

spec/syntax.md Outdated Show resolved Hide resolved
spec/syntax.md Outdated
@@ -495,6 +497,8 @@ Inside _patterns_,
whitespace is part of the translatable content and is recorded and stored verbatim.
Whitespace is not significant outside translatable text, except where required by the syntax.

***NOTE:*** Whitespace **_is_** significant in the `reserved` production and implementations need to be careful not to trim trailing whitespace from reserved sequences.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As an alternative solution, we could also tie our future hands a little, and explicitly make the trailing whitespace of reserved not significant. Realistically, I don't think that would cause any problems, especially as we're already considering trailing whitespace non-significant in other expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that, but we would need to change the ABNF. The problem is that all of our other productions are bounded in some way and the exterior whitespace is applied by the optional production [s]. Because reserved sequences are not delimited, there is no way to know where the whitespace belongs to the reserved or to the [s]. Future standardization would most likely use productions like name or nmtoken and the resulting trailing whitespace would "emerge" from the murky fog of reserved to be captured by [s]. Note that this is one of the reasons I originally proposed reserving the sigils to the function-like syntax.

I tend to agree with wanting to make the exterior whitespace not significant, in part because it interferes with user expectations. These two sequences are logically identical:

{      $foo :bar    }
{$foo :bar}

But these two are not:

{    $foo ~bar    }
{$foo ~bar}

I think we should fix this. Probably:

reserved = reserved-start reserved-body reserved-end
reserved-end = ; reserved-body less SP / HTAB/ CR / LF

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to distinguish the trailing spaces of reserved in the ABNF? Would it not be enough to include some statement like this in the text on reserved about future use?

Future uses of reserved expressions MUST NOT assign any significance to trailing whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should make the parser return the right tokens. See what you think of the revised abnf.

@macchiati
Copy link
Member

BTW, big fan of Semantic Line Breaks

@aphillips aphillips requested a review from eemeli April 18, 2023 09:07
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.

3 participants