-
-
Notifications
You must be signed in to change notification settings - Fork 36
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 self-closing MarkupEmpty element #273
Conversation
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 agree with the change but I have comments about the formal implementation, and I'd like to see another iteration please.
spec/message.ebnf
Outdated
@@ -21,6 +21,7 @@ Option ::= Name '=' (String | Nmtoken | Variable) | |||
/* Markup Tags */ | |||
MarkupStart ::= Name Option* | |||
MarkupEnd ::= '/' Name | |||
MarkupEmpty ::= Name Option* '/' |
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.
This isn't LL(1) because it conflicts with the MarkupStart
production. One way to fix this formally is something like the following (MarkupStart
may need to be renamed):
MarkupStart ::= Name Option* '/'?
MarkupEnd ::= '/' Name
This should be OK because the EBNF only describes the grammar, not the data model. Assuming we end up with ElementStart
, ElementEnd
, and ElementEmpty
in the data model, the parser can yield ElementStart
when MarkupStart
is missing the slash, and ElementEmpty
when the slash is present.
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're right. If keeping this syntax, it might make better to sense to have Markup
and MarkupEnd
in the syntax grammar, with Markup
having a data model attribute like isEmpty: boolean
determined by the presence (or not) of a trailing /
.
The discussion in #269 is also relevant, as it may lead to different markup for Markup.
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 realize that the lack of symmetry between Markup
and MarkupEnd
may be bothersome. How about a single Markup
production?
Markup ::= Name Option* '/'? | '/' Name
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.
While that certainly technically works, it does rather look like two separate constructs crammed into one.
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. EBNF requires a bit of formal accounting here and there to make it work, like the one I'm suggesting. Note that I only suggest to combine the grammar productions. The data model should have separate interfaces for all three element types.
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 updated the PR, merging MarkupStart and MarkupEmpty into one Markup element.
Co-authored-by: Stanisław Małolepszy <[email protected]>
Closing this, as the change proposed here has been obsoleted by subsequent conversation, esp. in #269. |
As discussed during this Monday's meeting, this PR adds the empty/standalone/self-closing
MarkupEmpty
element to the proposed spec, to represent elements that do not contain any body. This closes #238.