Add 0.5 comments support #109

zbraniecki · 2017-12-13T15:41:19Z

Implements a subset of projectfluent/fluent#58 required for limited Firefox x-channel compatibility across beta and stable.

stasm · 2017-12-13T21:08:42Z

fluent-syntax/src/ast.js

+    this.content = content;
+  }
+}
+
 export class Section extends Entry {


We should remove Section from ast.js.

stasm · 2017-12-13T21:09:27Z

fluent-syntax/src/ftlstream.js

+  //  0 - comment
+  //  1 - group comment
+  //  2 - resource comment
+  isPeekNextLineComment(level = -1, oldStyle) {


I know this is a temporary, but how about we rename oldStyle to zeroFourSyntax to make it explicit?

stasm · 2017-12-13T21:15:19Z

fluent-syntax/src/ast.js

  constructor(content) {
    super();
    this.type = 'Comment';
    this.content = content;
  }
 }

+export class Comment extends BaseComment {
+  constructor(content) {
+    super();


You could pass content to super and avoid setting it in again in the constructor here.

stasm · 2017-12-13T21:18:37Z

fluent-syntax/src/ftlstream.js

+        this.resetPeek();
+        return true;
+      }
+      if (level === 0 && this.currentPeekIs(' ')) {


It looks like this could use a while loop where you decrement level in each pass.

stasm · 2017-12-13T21:21:44Z

fluent-syntax/src/ast.js

  }
 }

-export class Comment extends Entry {
+export class BaseComment extends Entry {
  constructor(content) {
    super();
    this.type = 'Comment';


BaseComment

stasm · 2017-12-13T21:28:21Z

fluent-syntax/src/parser.js

+          ps.next();
+          if (ps.currentIs('#')) {
+            ps.next();
+            if (ps.currentIs('#')) {


Perhaps put this in a for loop iterating from 1 to level?

stasm · 2017-12-13T21:30:28Z

fluent-syntax/src/parser.js

@@ -117,7 +115,8 @@ export default class FluentParser {
  getEntry(ps) {
    let comment;

-    if (ps.currentIs('/')) {
+    if (ps.currentIs('/') ||
+      (ps.currentIs('#') && (ps.peekCharIs('#') || ps.peekCharIs(' ')))) {


Why does this peek one char ahead? Isn't a single # enough to branch into getComment?

Unfortunately, not.

key = Value #word

should not branch into getComment since the comment starting code is "# "|"## "|"### ".

I feel like that should throw a SyntaxError inside of getComment.

stasm · 2017-12-13T21:32:12Z

fluent-syntax/src/parser.js

@@ -179,7 +226,7 @@ export default class FluentParser {
    ps.expectChar(']');
    ps.expectChar(']');

-    return new AST.Section(symb, comment);
+    return new AST.Section(symb);


getSection should return a GroupComment with the section's comment or null (or undefined) if the comment is missing.

stasm · 2017-12-13T21:36:05Z

fluent-syntax/test/fixtures_structure/resource_comment.json

+    {
+      "type": "ResourceComment",
+      "annotations": [],
+      "content": "This is a resource wide comment\nIt's multiline"


Why is there no span here?

stasm · 2018-01-11T09:25:41Z

fluent-syntax/src/ast.js

-    this.name = name;
-    this.comment = comment;
+    this.type = 'GroupComment';
+    this.content = content;


You can also pass content to super() here and in ResourceComment.

stasm · 2018-01-11T10:21:09Z

fluent-syntax/src/ftlstream.js

+      return false;
+    }
+
+    let ptr = 0;


In other parts of this file, ptr is used to store a peek index. Perhaps rename this simply to i?

stasm · 2018-01-11T10:28:29Z

fluent-syntax/src/parser.js

      return this.getMessage(ps, comment);
    }

    if (comment) {
      return comment;
    }
+
+    if (ps.currentIs('[')) {
+      return this.getSection(ps);


Shouldn't we skip sections and return a GroupComment if comment is present?

stasm · 2018-01-11T10:28:57Z

fluent-syntax/src/parser.js

+    return new Comment(content);
+  }
+
+  getSection(ps) {


Perhaps rename to skipSection and don't return anything for it?

stasm · 2018-01-11T10:35:04Z

fluent-syntax/test/fixtures_behavior/section_starts_with_one_bracket.ftl

@@ -1,2 +0,0 @@
-[ This is a broken section ]]
-//~ ERROR E0003, pos 1, args "["


This should still throw a syntax error, no?

stasm · 2018-01-11T10:35:37Z

fluent-syntax/test/fixtures_behavior/section_with_no_nl_after_it.ftl

@@ -1 +0,0 @@
-[[ This is a correct section ]]


It would be good to still test that this parses alright, even if it parses to an empty AST at this point.

stasm · 2018-01-11T10:39:23Z

fluent-syntax/src/serializer.js

-        return `\n${comment}\n\n`;
-      }
+      case 'Comment':
+        return `\n${serializeComment(entry)}\n\n`;


I think we should have a flag on the FluentSerializer class which starts as isFirstEntry = true and only add those leading \n if it's false.

stasm

Let's bring back other tests from fixtures_behavior. Also, I think it should be possible to avoid the two node === null checks. See my comments inline.

stasm · 2018-01-11T16:19:36Z

fluent-syntax/src/parser.js

+      if (comment) {
+        return new AST.GroupComment(comment.content);
+      }
+      return null;


I might be missing something, but if you bring back the previous order of ifs, so first test for [ and then test for identifier start, you could get away without this return null here and just let the code flow to the next if.

You could then remove the node === null check in withSpan and parse.

and just let the code flow to the next if.

Then it'll return regular Comment for old style comment that was attached to a section, instead of GroupComment.

You could then remove the node === null check in withSpan and parse.

That's crashes if getEntry returns null (it does when section doesn't have a comment in 0.4 style).

I mean something like this:

getEntry(ps) { let comment; if (ps.currentIs('/') || ps.currentIs('#')) { comment = this.getComment(ps); // The Comment content doesn't include the trailing newline. Consume // this newline here to be ready for the next entry. undefined stands // for EOF. ps.expectChar(ps.current() ? '\n' : undefined); } if (ps.currentIs('[')) { this.skipSection(ps); if (comment) { return new AST.GroupComment(comment.content); } } if (ps.isIDStart() && (!comment || comment.type === 'Comment')) { return this.getMessage(ps, comment); } if (comment) { return comment; } throw new ParseError('E0002'); }

Then you never return null.

I do in case the file is:

[[ This is a section ]]

which is one of our tests :(

What do you prefer here?

Ah, good point, thanks. Let's keep the tests.

Can we at least remove the node === null check in withSpans? I think we only return null from getEntry which isn't decorated with withSpans because it handles spans in an own custom way.

stasm · 2018-01-11T16:20:49Z

fluent-syntax/src/serializer.js

    }

    for (const entry of resource.body) {
      if (entry.types !== 'Junk' || this.withJunk) {
        parts.push(this.serializeEntry(entry));
+        this.hasEntries = true;


I would avoid setting it in every pass of the loop and only do it if hasEntries is false.

stasm · 2018-01-11T16:31:17Z

fluent-syntax/src/serializer.js

+        return `\n${serializeGroupComment(entry)}\n\n`;
+      case 'ResourceComment':
+        if (this.hasEntries) {
+          return `${serializeResourceComment(entry)}\n\n`;


This should start with \n.

stasm · 2018-01-11T17:02:46Z

fluent-syntax/src/parser.js

+        continue;
+      }
+
+      if (entry.type === 'Comment' && entry.oldStyle && entries.length === 0) {


I don't see oldStyle anywhere else in the PR.

stasm

Thanks, @zbraniecki, this looks great! Please don't remove any test fixtures in this PR. The parser should still report syntax errors on broken sections.

stasm · 2018-01-11T19:09:00Z

fluent-syntax/src/parser.js

+      if (comment) {
+        return new AST.GroupComment(comment.content);
+      }
+      return null;


Ah, good point, thanks. Let's keep the tests.

Can we at least remove the node === null check in withSpans? I think we only return null from getEntry which isn't decorated with withSpans because it handles spans in an own custom way.

zbraniecki requested a review from stasm December 13, 2017 15:41

zbraniecki force-pushed the 05-comments branch 4 times, most recently from e089369 to 82533eb Compare December 13, 2017 21:27

stasm suggested changes Dec 13, 2017

View reviewed changes

zbraniecki force-pushed the 05-comments branch from 82533eb to 301f2b9 Compare January 10, 2018 23:00

zbraniecki requested a review from stasm January 10, 2018 23:21

stasm suggested changes Jan 11, 2018

View reviewed changes

zbraniecki force-pushed the 05-comments branch from 301f2b9 to 4f78d42 Compare January 11, 2018 15:56

zbraniecki requested a review from stasm January 11, 2018 15:56

stasm suggested changes Jan 11, 2018

View reviewed changes

zbraniecki force-pushed the 05-comments branch from 4f78d42 to b2e8b7f Compare January 11, 2018 17:19

zbraniecki requested a review from stasm January 11, 2018 17:19

stasm approved these changes Jan 11, 2018

View reviewed changes

zbraniecki force-pushed the 05-comments branch from b2e8b7f to ea095c9 Compare January 11, 2018 19:30

Add 0.5 comments support

89fb032

zbraniecki force-pushed the 05-comments branch from ea095c9 to 89fb032 Compare January 11, 2018 19:32

zbraniecki merged commit 92e77df into projectfluent:master Jan 11, 2018

zbraniecki mentioned this pull request Jan 26, 2018

Update to fluent syntax 0.5 #112

Closed

4 tasks

zbraniecki deleted the 05-comments branch May 14, 2018 09:47

		@@ -1,2 +0,0 @@
		[ This is a broken section ]]
		//~ ERROR E0003, pos 1, args "["

Add 0.5 comments support #109

Add 0.5 comments support #109

Uh oh!

Conversation

zbraniecki commented Dec 13, 2017

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!

Choose a reason for hiding this comment

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!

stasm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!