Refactor ftl parsing to use fluent-syntax/compat. #1888

EnTeQuAk · 2018-03-08T08:32:41Z

Also fixes package.json to use and define correct webpack dependencies
Updates yarn.lock

~~Do not merge yet, blocked by projectfluent/fluent.js#164~~

EnTeQuAk · 2018-03-08T09:03:29Z

.travis.yml

@@ -7,6 +7,8 @@ script:
  - yarn run test-ci
  - yarn run build
  - yarn run lint
+before_install:
+  - yarn config set ignore-engines true


Temporary until projectfluent/fluent.js#164 get's merged

Isn't this actually a better temporary solution to this problem than projectfluent/fluent.js#164? Assuming the longer term solution is to drop the support for Node 6 in addons-linter.

No, it's not since the linter is being installed by users via web-ext - https://github.com/mozilla/web-ext and a strict engine in package.json doesn't allow the user to install the linter via npm or yarn.

stasm · 2018-03-08T09:56:25Z

.travis.yml

@@ -7,6 +7,8 @@ script:
  - yarn run test-ci
  - yarn run build
  - yarn run lint
+before_install:
+  - yarn config set ignore-engines true


Isn't this actually a better temporary solution to this problem than projectfluent/fluent.js#164? Assuming the longer term solution is to drop the support for Node 6 in addons-linter.

stasm · 2018-03-08T09:57:15Z

src/parsers/fluent.js

@@ -18,30 +19,47 @@ export default class FluentParser {
    this.isValid = null;
  }

+  getLineAndColumnFromSpan(span) {


This looks like a job for lineOffset and columnOffset exported by fluent-syntax :)

https://github.com/projectfluent/fluent.js/blob/fee9cbf347c4edecaaf60f757da6c515110f2356/fluent-syntax/src/index.js#L17-L36

Ah, I didn't see those helpers yet. The code is quite similar, I'll use the upstream version then, thanks for the hint!

LinusU · 2018-03-08T11:26:11Z

package.json

@@ -79,7 +79,8 @@
    "request": "2.83.0",
    "sinon": "4.4.2",
    "tar": "4.4.0",
-    "webpack": "3.10.0",
+    "webpack": "4.0.1",


It seems like it was actually webpack 3.x that was used before, webpack-grunt doesn't have webpack 4.x release supported yet (it's merged into master though so should only be a matter of time 🎉)

I also addressed this in #1889, but maybe this has all the changes required to go to webpack 4.x?

Just merged or going to merge the removal of webpack-grunt dependency so we'll be able to update to webpack 4.x soon. But step by step…

I'll back out the package.json changes here anyway.

This now got rebased to fluent-syntax 0.6.5 which re-adds node 6 LTS compatibility. Fixes #1789

EnTeQuAk · 2018-03-08T14:56:47Z

Rebased to master, removed earlier hacks, rebased to fluent-syntax 0.6.5 which adds node6 support.

@rpl if you have a moment, this should be ready for review.

EnTeQuAk · 2018-03-08T15:12:22Z

tests/parsers/test.fluent.js

-          },
-        ],
-      },
-    });


I primarily removed these since I essentially only copy the upstream tree into parsedData and that tree is huge and would bloat up our tests. There is one test that verifies things at a very basic level so I thought that has to suffice.

tofumatt

Works for me. As mentioned I don't really know fluent all that well but the code seems fine save for my magic-constant-phobia.

Maybe worth a look from someone else who knows fluent better but then if it doesn't break all the other tests I guess it's 👍

tofumatt · 2018-03-13T17:57:17Z

src/parsers/fluent.js

-      this.parsedData[id] = entries[id];
-    });
+    resource.body.forEach((entry) => {
+      if (entry.type === 'Junk') {


If fluent-syntax provides a constant for this it'd be nice, magic constants just make me scared.

Unfortunately not, I might push a follow-up PR to clean this up a bit and think about better tests.

It provides a type you can test against instanceof - https://github.com/projectfluent/fluent.js/blob/master/fluent-syntax/src/ast.js#L240

👍 on importing the Junk class and turning this into entry instanceof Junk in a follow up as @zbraniecki pointed out.

tofumatt · 2018-03-13T17:59:04Z

tests/parsers/test.fluent.js

-        val: undefined,
-      },
-    });
+    expect(parser.parsedData.key67.attributes[0].value.elements[0].value).toEqual(


These tests read foreign to me without context but I have no idea how fluent works... I'm gonna trust you on them 😄

EnTeQuAk · 2018-03-13T18:05:10Z

I'm going to merge this, if there's any more comments I'm very happy to update the implementation but to land this this week I'm merging and releasing a new version of the linter for now.

It's not breaking the tests and actually fixing something so it can't be too bad 🙈

rpl · 2018-03-13T16:42:31Z

src/parsers/fluent.js

        const errorData = {
          ...messages.FLUENT_INVALID,
          file: this.filename,
-          // normalize newlines and flatten the message a bit.
-          description: error.message.replace(/(?:\n(?:\s*))+/g, ' '),
+          description: entry.annotations[0].message,


We have already assigned entry.annotations[0] to annotation at line 36, this could just be annotation.message.

rpl · 2018-03-13T16:44:41Z

src/parsers/fluent.js

        };

        this.collector.addError(errorData);
-      });
-    }
+      } else if (entry.id !== undefined) {


if entry.id can be both null or undefined this should be entry.id != null.

rpl · 2018-03-13T18:22:19Z

src/parsers/fluent.js

-      this.parsedData[id] = entries[id];
-    });
+    resource.body.forEach((entry) => {
+      if (entry.type === 'Junk') {


👍 on importing the Junk class and turning this into entry instanceof Junk in a follow up as @zbraniecki pointed out.

EnTeQuAk commented Mar 8, 2018

View reviewed changes

stasm reviewed Mar 8, 2018

View reviewed changes

LinusU reviewed Mar 8, 2018

View reviewed changes

EnTeQuAk mentioned this pull request Mar 8, 2018

fix(package): fix conflicting dependencies #1889

Merged

2 tasks

Refactor ftl parsing to use fluent-syntax.

491f9ea

This now got rebased to fluent-syntax 0.6.5 which re-adds node 6 LTS compatibility. Fixes #1789

EnTeQuAk force-pushed the 1789-use-fluent-syntax branch from 7b43c08 to 491f9ea Compare March 8, 2018 14:49

Use upstream lineOffset and columnOffset

ef123e2

EnTeQuAk force-pushed the 1789-use-fluent-syntax branch from 0e18d2d to ef123e2 Compare March 8, 2018 15:05

EnTeQuAk commented Mar 8, 2018

View reviewed changes

EnTeQuAk requested a review from rpl March 8, 2018 17:14

tofumatt approved these changes Mar 13, 2018

View reviewed changes

EnTeQuAk merged commit 0c7870c into master Mar 13, 2018

EnTeQuAk deleted the 1789-use-fluent-syntax branch March 13, 2018 18:05

rpl reviewed Mar 13, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ftl parsing to use fluent-syntax/compat. #1888

Refactor ftl parsing to use fluent-syntax/compat. #1888

EnTeQuAk commented Mar 8, 2018 •

edited

Loading

EnTeQuAk Mar 8, 2018

stasm Mar 8, 2018

EnTeQuAk Mar 8, 2018

stasm Mar 8, 2018

stasm Mar 8, 2018

EnTeQuAk Mar 8, 2018

LinusU Mar 8, 2018

EnTeQuAk Mar 8, 2018

EnTeQuAk commented Mar 8, 2018 •

edited

Loading

EnTeQuAk Mar 8, 2018

tofumatt left a comment

tofumatt Mar 13, 2018

EnTeQuAk Mar 13, 2018

zbraniecki Mar 13, 2018

rpl Mar 13, 2018

tofumatt Mar 13, 2018

EnTeQuAk commented Mar 13, 2018

rpl Mar 13, 2018

rpl Mar 13, 2018

rpl Mar 13, 2018

Refactor ftl parsing to use fluent-syntax/compat. #1888

Refactor ftl parsing to use fluent-syntax/compat. #1888

Conversation

EnTeQuAk commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnTeQuAk commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnTeQuAk commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnTeQuAk commented Mar 8, 2018 •

edited

Loading

EnTeQuAk commented Mar 8, 2018 •

edited

Loading