-
Notifications
You must be signed in to change notification settings - Fork 77
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 private messages #130
Add private messages #130
Conversation
@@ -45,6 +46,14 @@ function getErrorMessage(code, args) { | |||
return 'Expected literal'; | |||
case 'E0015': | |||
return 'Only one variant can be marked as default (*)'; | |||
case 'E0016': |
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.
fluent-rs uses smaller errors numbers for these because it seems to be missing a few earlier ones.
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'd like to make error naming part of the developer productivity review after 0.7.
@@ -693,17 +718,25 @@ export default class FluentParser { | |||
throw new ParseError('E0014'); | |||
} | |||
|
|||
if (ch === '$') { |
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 moved the $
check to the top as this will likely be the most common literal.
@@ -28,7 +28,7 @@ export default class FluentSerializer { | |||
} | |||
|
|||
for (const entry of resource.body) { | |||
if (entry.types !== 'Junk' || this.withJunk) { | |||
if (entry.type !== 'Junk' || this.withJunk) { |
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.
A nasty typo but fortunately didn't impact many tests. It made the serializer serialize Junks verbatim as they appear in the parsed text. Input magically always matched the output...
@@ -886,7 +944,7 @@ class RuntimeParser { | |||
|
|||
if ((cc >= 97 && cc <= 122) || // a-z | |||
(cc >= 65 && cc <= 90) || // A-Z | |||
cc === 95 || cc === 47 || cc === 91) { // _/[ | |||
cc === 47 || cc === 91) { // /[ |
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 think this shouldn't check for /
nor [
anymore? I'll fix it in a follow-up
ae5dee4
to
6328455
Compare
const cc = this.currentPeek().charCodeAt(0); | ||
const isDigit = cc >= 48 && cc <= 57; // 0-9 | ||
this.resetPeek(); | ||
return isDigit; |
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.
what's the value of this change?
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.
It makes isNumberStart
consistent with isMessageIDStart
and more importantly, it considers the char after the leading -
. The previous version would return true
upon seeing a dash right away and more checking was required in getLiteral
.
if (allowPrivate && this.currentIs('-')) { | ||
this.next(); | ||
return '-'; | ||
} |
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 should be able to return this.next();
here.
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.
We want to return current
here but first, we advance the index
.
6328455
to
9e64b69
Compare
9e64b69
to
99e9f26
Compare
Implement projectfluent/fluent#62.