Require = after the id (but not really) #132

stasm · 2018-01-23T22:55:57Z

In Fluent Syntax 0.5, the identifier must be followed by an equals sign (=)
even if the message doesn't have a value.

key =
    .attribute = Attribute Value

For some time fluent.js will allow both the new (0.5) and the old 0.4 syntax,
which means that the following will still parse but the Serializer will insert
the = after the key.

key
    .attribute = Attribute Value

This patch also fixes Bug 1406880 - Decide where multiline Pattern spans should
start. The patterns now start when their first

key =
    Value
    ^---- Pattern span start

Lastly, this patch forbids null variant values. Empty values should be written
with the {""} idiom:

key =
    {
       *[valid] {""}
        [invalid]
    }

stasm · 2018-01-23T22:59:44Z

@zbraniecki This isn't ready to land yet; I'm still working on implementing the same thing in the runtime parser. The full AST parser is good to go, however. Perhaps you'd like to take a look?

I had to rework most of getMessage because it used to always go to getPattern upon seeing a = which isn't the proper behavior for Syntax 0.5.

stasm · 2018-01-24T14:31:26Z

fluent-syntax/src/ftlstream.js

@@ -234,7 +261,7 @@ export class FTLParserStream extends ParserStream {
    return false;
  }

-  isPeekNextNonBlankLinePattern() {
+  isPeekNextLinePatternStart() {


All of the isPeekNextLine* methods only consider non-blank lines so I renamed this one to match the naming of all others.

stasm · 2018-01-24T14:43:31Z

fluent-syntax/src/ftlstream.js

-        break;
-      }
-      this.next();
+  maybeExpectIndent() {


This skips inline whitespace as well as new lines but the latter only if there's indentation afterwards. It throws if there isn't. (I'll put this in a comment in the code.)

I ended up removing this method and replacing it with ps.isPeekPatternStart checks combined with a simple ps.skipIndent.

stasm · 2018-01-24T14:45:44Z

fluent-syntax/src/parser.js

@@ -459,18 +460,12 @@ export default class FluentParser {
    const elements = [];
    ps.skipInlineWS();

-    // Special-case: trim leading whitespace and newlines.
-    if (ps.isPeekNextNonBlankLinePattern()) {


This isn't needed any more because calls to getPattern are guarded by isPeekPatternStart, followed by skipIndent. This effectively fixes Bug 1406880 - Decide where multiline Pattern spans should start.

stasm · 2018-01-24T14:47:42Z

fluent-syntax/test/fixtures_behavior/attribute_with_empty_pattern.ftl

@@ -1,2 +1,3 @@
 key = Value
    .label =
+//~ ERROR E0003, pos 25, args " "


This isn't terribly useful but kind of makes sense (the parser expects an indent after the newline in line 2). Let's try to improve the error in the future.

stasm · 2018-01-24T14:50:43Z

fluent/src/parser.js

-    let val;
-
-    if (ch === '=') {
+    if (this._source[this._index] === '=') {
      this._index++;


I just skip = here which means things like a b c will be parsed as a = b c. I'd prefer to fix it later on when we remove the support for Syntax 0.4.

stasm · 2018-01-24T15:13:33Z

fluent/src/parser.js

-        // This is a fast-path for the most common
-        // case of `key = Value` where the value
-        // is in the same line as the key.
-        val = this.getPattern();


This fast path wasn't needed because it's essentially almost the same as what getPattern does. Overall, I'm seeing a slight speed-up with my patch, around -7%.

stasm · 2018-01-24T16:46:54Z

fluent/test/fixtures_structure/attribute_with_empty_pattern.json

+    "val": "Value 1",
+    "attrs": {
+      "attr": {
+        "val": null


These are all syntax errors in the tooling parser. In the runtime parser I chose to parse them as null values, at least for now, to reduce the complexity of this PR and to keep the performance benefits.

stasm · 2018-01-24T16:49:41Z

@zbraniecki This is now ready for your review. I wrestled a bit with the runtime parser; I think I finally managed to get it acceptably right :) Let me know what you think, thanks!

zbraniecki · 2018-01-24T18:21:46Z

fluent/src/resolver.js

@@ -294,7 +294,8 @@ function Type(env, expr) {
    }
    case undefined: {
      // If it's a node with a value, resolve the value.
-      if (expr.val !== undefined) {
+      // eslint-disable-next-line eqeqeq
+      if (expr.val != null) {


why != instead of !==?

To check both for null and undefined; see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness.

Just to make it clear in case it came across wrong: I don't suppose you don't know the difference :) Just wanted to link to the Loose comparison matrix for reference of what != null evaluates to.

since we usually use strict comparison, can you add a comment on why we use a different strategy here?

I'll just use the more explicit double comparison. Can't hurt the readability. Thanks!

zbraniecki

lgtm!

In Fluent Syntax 0.5, the identifier must be followed by an equals sign (=) even if the message doesn't have a value. key = .attribute = Attribute Value For some time fluent.js will allow both the new (0.5) and the old 0.4 syntax, which means that the following will still parse but the Serializer will insert the = after the key. key .attribute = Attribute Value This patch also fixes Bug 1406880 - Decide where multiline Pattern spans should start. The patterns now start when their first key = Value ^---- Pattern span start Lastly, this patch forbids null variant and attribute values in the tooling parser. Empty values should be written with the {""} idiom: key = { *[valid] {""} [invalid] } The runtime parser parses them as {val: null} for performance reasons.

stasm force-pushed the require-equals branch 2 times, most recently from 9a8294d to 6dc5dc9 Compare January 24, 2018 12:36

stasm commented Jan 24, 2018

View reviewed changes

stasm force-pushed the require-equals branch 4 times, most recently from 768991e to bd23dd8 Compare January 24, 2018 16:44

stasm commented Jan 24, 2018

View reviewed changes

stasm requested a review from zbraniecki January 24, 2018 16:48

zbraniecki reviewed Jan 24, 2018

View reviewed changes

zbraniecki approved these changes Jan 24, 2018

View reviewed changes

stasm force-pushed the require-equals branch from bd23dd8 to c911033 Compare January 24, 2018 18:53

stasm merged commit 4d1a41f into projectfluent:master Jan 24, 2018

stasm deleted the require-equals branch January 24, 2018 18:57

zbraniecki mentioned this pull request Jan 26, 2018

Update to fluent syntax 0.5 #112

Closed

4 tasks

stasm mentioned this pull request Jan 29, 2018

Fix parsing of a trailing = #118

Closed

Require = after the id (but not really) #132

Require = after the id (but not really) #132

Uh oh!

Conversation

stasm commented Jan 23, 2018

Uh oh!

stasm commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stasm Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stasm commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zbraniecki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stasm Jan 24, 2018 •

edited

Loading