Skip to content

Conversation

@stasm
Copy link
Contributor

@stasm stasm commented Nov 28, 2018

Implement part 7 of #303.

@stasm stasm mentioned this pull request Nov 28, 2018
15 tasks
@stasm stasm changed the base branch from master to zeroeight November 29, 2018 09:04
@stasm
Copy link
Contributor Author

stasm commented Nov 29, 2018

This PR depends on #313 which hasn't been merged yet. @Pike, if you want to review this first, the compare view might be helpful. Thanks.

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.

Just a nit about checking u and U twice, otherwise, nice, r=me.

for (let i = 0; i < 4; i++) {
const ch = ps.takeHexDigit();
getUnicodeEscapeSequence(ps, u, digits) {
ps.expectChar(u);
Copy link
Contributor

Choose a reason for hiding this comment

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

We already checked this in the switch statement, I'd just ps.next() here, and drop the u argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep expectChar. In fact, I'm planning a clean up after 0.8 where we get rid of most of ps.next(). I always seem to forget what it's supposed to consume, something expectChar is very explicit about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and we need the u arg for the error message. That's the real reason why it's there.

For compatibility with the reference parser which used to use the /./
regex, the tooling parser used to recognize additional EOL characters in
Comments. This has since been fixed in the reference parser:

    projectfluent/fluent@2d224cb
@stasm stasm merged commit 730f005 into projectfluent:zeroeight Nov 30, 2018
@stasm stasm deleted the zeroeight-part7 branch November 30, 2018 12:45
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