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

Treat backslash as normal char in TextElements #181

Merged
merged 5 commits into from
Oct 30, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Oct 16, 2018

Fix #123.

@stasm stasm force-pushed the remove-escapes-from-text branch 2 times, most recently from 59c2cda to 11b5a8e Compare October 16, 2018 08:08
@stasm stasm force-pushed the remove-escapes-from-text branch from a782d22 to 0640785 Compare October 23, 2018 16:15
@stasm stasm requested a review from Pike October 26, 2018 13:30
@stasm stasm force-pushed the remove-escapes-from-text branch from 148883c to 2356656 Compare October 29, 2018 11:30
Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

The general idea is right. Also the tests are OK.

I think there's symmetry coming between { and } which I'd expect to be expressed differently than right now, but maybe I'm off.

The ebnf is rather confusing in its grouping and the comments, sadly. Can you shuffle and group/paragraph that to be more, well, grouped and paragraphed?

/* Double quote and backslash need to be escaped in string literals. */
special_quoted_char ::= "\""
                      | "\\"
text_char           ::= any_char - special_text_char
/* Indented text may not start with characters which mark its end. */
indented_char       ::= text_char - "}" - "[" - "*" - "."
special_escape      ::= "\\" special_quoted_char
unicode_escape      ::= "\\u" /[0-9a-fA-F]{4}/

is really hard to read right now, in particular as the comments earlier usually denote block comments. And here, the comments don't make any sense as block comments.

syntax/grammar.mjs Show resolved Hide resolved
@stasm stasm requested a review from Pike October 30, 2018 07:24
@stasm stasm merged commit d5e12e7 into projectfluent:master Oct 30, 2018
@stasm stasm deleted the remove-escapes-from-text branch October 30, 2018 11:14
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.

2 participants