Add AST.Placeable #64

stasm · 2017-08-21T13:32:06Z

stasm

@zbraniecki, I think this is ready for your review.

stasm · 2017-08-21T14:08:21Z

fluent-syntax/src/parser.js

+        break;
+      }
+
+      if (ch === '{') {


Should we make the closing brace } special as well? It's not special right now, i.e. it doesn't need to be escaped to be part of a TextElement if it's not matched. This makes the following FTL valid: key = { foo } }, with <space>} being a TextElement.

I'd prefer to not enforce escaping if it's not necessary

Well, my question is then: do you think it's necessary? No syntax error in key = { foo } } is a bit weird to me.

sorry, I should have state my opinion. I don't believe that it's necessary. I think it only looks weird because of the semantic meaning you give to } when seen in this context.
I'd argue that:

foo = Bar = Baz

looks equally weird. :)

OK, that's a good point, thanks!

stasm · 2017-08-21T14:53:58Z

fluent-syntax/src/util.js

@@ -0,0 +1,3 @@
+export function includes(arr, elem) {
+  return arr.indexOf(elem) > -1;


String.prototype.includes is quite well-supported but since this quasi-polyfill is so small and simple I thought there was not point in artificially excluding IE11. Note that our compat builds don't polyfill includes automatically.

could we get a compat-utils that are included only in the compat build? :) I think it would be nice to invest a bit of our time to design a build system for fluent which does not penalize evergreens for the sins of their ancestors. Although I agree that in this case the polyfill is small :)

We could change String.prototype and Array.prototype and somehow do this only for the compat build. I don't think it's worth it though. Almost all of our needs are covered by the current pipeline. includes is a rare exception.

Nah, let's do what you did in your patch.

stasm · 2017-08-21T15:00:55Z

fluent-syntax/src/parser.js

      return new AST.SelectExpression(null, variants);
    }

+    ps.skipInlineWS();


I think this should be skipWS really to allow newlines after the opening { but I couldn't quite track down a few test failures. I went for skipInlineWS for now to stay close to the current implementation. We'll need a whole new PR to allow newlines in different places anyways.

I filed #67 to track this.

stasm · 2017-08-21T15:02:33Z

fluent-syntax/test/fixtures_behavior/multiline_with_non_empty_first_line.ftl

@@ -1,3 +1,2 @@
 key = Value
    Value 2
-//~ ERROR E0002, pos 12


This is allowed by the EBNF and should not be a syntax error. (It might be a linting error however.)

stasm · 2017-08-21T19:05:22Z

fluent-syntax/src/ftlstream.js

    return ((cc >= 48 && cc <= 57) || cc === 45); // 0-9
  }

-  isPeekNextLineIndented() {


This wasn't used anywhere.

stasm · 2017-08-21T19:06:13Z

fluent-syntax/src/ftlstream.js


  skipToNextEntryStart() {
-    while (this.next()) {
+    while (this.ch) {


When the syntax error happens exactly on the newline character, this.next() would overshoot the beginning of the entry on the next line, if present.

stasm · 2017-08-21T19:06:56Z

fluent-syntax/src/index.js

+  // Find the last line break starting backwards from the index just before
+  // pos.  This allows us to correctly handle ths case where the character at
+  // pos  is a line break as well.
+  const fromIndex = pos - 1;


Without this, columnOffset was returning 0 when the character at pos was a new line.

stasm · 2017-08-21T19:08:04Z

fluent-syntax/src/parser.js

-    return new AST.Pattern(elements);
+  getPlaceable(ps) {
+    ps.expectChar('{');
+    const expression = this.getExpression(ps);


getExpression should know how to handle nested placeables. I filed #66 for this.

stasm · 2017-08-21T19:09:03Z

fluent-syntax/src/serializer.js

+
+  switch (expr.type) {
+    case 'Placeable':
+      return `{${serializePlaceable(expr)}}`;


I can't test this yet because the parser doesn't support nested Placeables.

zbraniecki

lgtm!

zbraniecki · 2017-08-22T06:26:09Z

fluent-syntax/src/parser.js

+    // Special-case: trim leading whitespace and newlines.
+    if (ps.isPeekNextLinePattern()) {
+      ps.skipBlankLines();
+      ps.skipInlineWS();


it would be nice if we could just peek to the first character that is non-WS and if it's a pattern start, skip to peek here, instead of resetting and skipping again, but since it's not a runtime parser, I don't think it's a blocker.

Yeah, I kept this to stay in line with other isPeek... methods which always reset the peek. We could try changing them such that they skipToPeek before returning true, although that would still need a bit of hand-holding in isPeekNextLineVariantStart.

- Implement Fluent Syntax 0.5. - Add support for terms. - Add support for `#`, `##` and `###` comments. - Remove support for tags. - Add support for `=` after the identifier in message and term defintions. - Forbid newlines in string expressions. - Allow trailing comma in call expression argument lists. In fluent-syntax 0.6.x the new Syntax 0.5 is supported alongside the old Syntax 0.4. This should make migrations easier. `FluentParser` will correctly parse Syntax 0.4 comments (prefixed with `//`), sections and message definitions without the `=` after the identifier. The one exception are tags which are no longer supported. Please use attributed defined on terms instead. `FluentSerializer` always serializes using the new Syntax 0.5. - Add `AST.Placeable` (#64) Added in Syntax Spec 0.4, `AST.Placeable` provides exact span data about the opening and closing brace of placeables. - Expose `FluentSerializer.serializeExpression`. (#134) - Serialize standalone comments with surrounding white-space. - Allow blank lines inside of messages. (#76) - Trim trailing newline from Comments. (#77)

stasm mentioned this pull request Aug 21, 2017

Add the Placeable node projectfluent/python-fluent#17

Merged

stasm force-pushed the ast-placeable branch 5 times, most recently from 1f43bf5 to 99b84b7 Compare August 21, 2017 19:04

stasm requested a review from zbraniecki August 21, 2017 19:09

stasm commented Aug 21, 2017

View reviewed changes

stasm force-pushed the ast-placeable branch from 99b84b7 to 861f194 Compare August 21, 2017 19:13

zbraniecki approved these changes Aug 22, 2017

View reviewed changes

Add AST.Placeable

90561ab

stasm force-pushed the ast-placeable branch from 861f194 to 90561ab Compare August 23, 2017 14:01

stasm merged commit 7da89a6 into projectfluent:master Aug 23, 2017

stasm deleted the ast-placeable branch August 23, 2017 14:22

stasm mentioned this pull request Sep 16, 2017

fluent-syntax: Add syntax for plain text interpolation #74

Closed

		@@ -0,0 +1,3 @@
		export function includes(arr, elem) {
		return arr.indexOf(elem) > -1;

Add AST.Placeable #64

Add AST.Placeable #64

Uh oh!

Conversation

stasm commented Aug 21, 2017

Uh oh!

stasm left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants