Skip to content

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.

@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