-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix whitespace conformance to match UAX31 (including permitting LRM/RLM) #673
Closed
Closed
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
b387301
Allow LRM in whitespace
aphillips ae712d7
Update syntax.md
aphillips c556ecf
Make 3a-2 definition consistent with requirements
aphillips 8558eef
Update spec/syntax.md
aphillips d830550
Replace `[s]` with owsp production
aphillips 42b7744
`s` != `wsp`
aphillips af75420
Update syntax.md
aphillips 6a6dc70
Update message.abnf
aphillips db5e97c
Update spec/syntax.md
aphillips 3fede7a
Update spec/syntax.md
aphillips 586a4d6
Update spec/syntax.md
aphillips 502e1fe
Update spec/syntax.md
aphillips d7af5eb
Address literals
aphillips 68f32f8
Fix converting some `s` productions to `wsp`
aphillips e36d01d
Add bidi isolate support
aphillips 5609dc0
Missing one "x"
aphillips e74b34c
Add a warning/tech preview feedback note
aphillips 9557849
Update spec/syntax.md
aphillips ec369ec
Update spec/syntax.md
aphillips 2532206
Address @macchiati's comment
aphillips f985860
Update spec/syntax.md
aphillips 52c5c86
Update spec/syntax.md
aphillips 4a6c96c
Fix up @machiatti's suggested text
aphillips 19864c3
Address @mihnita's suggestion, fix expression brackets
aphillips 27b9e42
Make syntax.md make abnf
aphillips 00e3796
Merge branch 'main' into aphillips-allow-lrm
aphillips File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't see why there is the restriction on unquoted literals. Shouldn't it be any literal? That is, anywhere a unquoted literal can appear, and unquoted one can. So it seems like either both (aka just 'literal') or neither can appear.
I could match on:
Where X is a RTL character, or on
In both cases the result is jumbled (disregard the direction of the fake braces, the tool just reorders.
Now, in this case I could put LRMs before the first ⎸ and after the second, because both positions allow whitespace. Are there circumstances around literals where it makes a difference in the insertability of LRM/RLM because of the quoting?
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.
Other than that, the changes look good to me. Note that if there are any issues in the WG about this, we refrain from these changes until after the v45 release, just leaving a note that we're looking at the bidi ordering issues...
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. The key thing I think is to remind tool writers not to quote the mark onto the value.
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.
The changes are to the syntax and I think important enough to merit doing the change now--the better to stabilize the syntax. It does represent a relaxation of what is allowed in free whitespace. I would like to avoid having a lot of Tech Preview implementations reject bidi-friendlier messages in the fall.
OTOH, it does represent a departure from how we set up the
s
production.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, for a LTR reading, you want to put LRM around any 'element' that could contain RTL characters. Each literal being matched, literals in option values, etc. So in
You want to insert like:
Of course:
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 definitely do not want the LRMs in the text part of the message, which is where the ones after the
{{
and before the}}
are. Instead there should be an LRM following the pattern so that any keys in the next variant aren't reordered:Agreed that this is a job for tools.
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, yes, imediately outside {{ and }}, not inside.