Review comments. Mostly camelCases. And Object.create

Pike · Pike · commit dbe7134289dc · 2019-03-13T20:03:41.000+01:00
This is different to python-fluent, as .equals doesn't ignore ordering
of Attributes and Variants anymore. Issue on python-fluent is filed.

Added an explicit test to ensure that SomeNode.clone
creates the same class.
diff --git a/fluent-syntax/src/ast.js b/fluent-syntax/src/ast.js
@@ -33,19 +33,12 @@ export class BaseNode {
         if (thisVal.length !== otherVal.length) {
           return false;
         }
-        // Sort elements of order-agnostic fields to ensure the
-        // comparison is order-agnostic as well. Annotations should be
-        // here too but they don't have sorting keys.
-        if (["attributes", "variants"].indexOf(fieldName) >= 0) {
-          thisVal.sort(sorting_key_compare);
-          otherVal.sort(sorting_key_compare);
-        }
-        for (let i = 0, ii = thisVal.length; i < ii; ++i) {
-          if (!scalars_equal(thisVal[i], otherVal[i], ignoredFields)) {
+        for (let i = 0; i < thisVal.length; ++i) {
+          if (!scalarsEqual(thisVal[i], otherVal[i], ignoredFields)) {
             return false;
           }
         }
-      } else if (!scalars_equal(thisVal, otherVal, ignoredFields)) {
+      } else if (!scalarsEqual(thisVal, otherVal, ignoredFields)) {
         return false;
       }
     }
@@ -62,31 +55,21 @@ export class BaseNode {
       }
       return value;
     }
-    const clone = Object.create(this);
+    const clone = Object.create(this.constructor.prototype);
     for (const prop of Object.keys(this)) {
       clone[prop] = visit(this[prop]);
     }
     return clone;
   }
 }
 
-function scalars_equal(thisVal, otherVal, ignoredFields) {
+function scalarsEqual(thisVal, otherVal, ignoredFields) {
   if (thisVal instanceof BaseNode) {
     return thisVal.equals(otherVal, ignoredFields);
   }
   return thisVal === otherVal;
 }
 
-function sorting_key_compare(left, right) {
-  if (left.sorting_key < right.sorting_key) {
-    return -1;
-  }
-  if (left.sorting_key === right.sorting_key) {
-    return 0;
-  }
-  return 1;
-}
-
 /*
  * Base class for AST nodes which can have Spans.
  */
@@ -267,7 +250,7 @@ export class Attribute extends SyntaxNode {
     this.value = value;
   }
 
-  get sorting_key() {
+  get sortingKey() {
     return this.id.name;
   }
 }
@@ -281,7 +264,7 @@ export class Variant extends SyntaxNode {
     this.default = def;
   }
 
-  get sorting_key() {
+  get sortingKey() {
     if (this.key instanceof NumberLiteral) {
       return this.key.value;
     }
diff --git a/fluent-syntax/src/visitor.js b/fluent-syntax/src/visitor.js
@@ -12,12 +12,11 @@ export class Visitor {
     if (!(node instanceof BaseNode)) {
       return;
     }
-    const nodename = node.type;
-    const visit = this[`visit_${nodename}`] || this.generic_visit;
+    const visit = this[`visit${node.type}`] || this.genericVisit;
     visit.call(this, node);
   }
 
-  generic_visit(node) {
+  genericVisit(node) {
     for (const propname of Object.keys(node)) {
       this.visit(node[propname]);
     }
@@ -32,12 +31,11 @@ export class Transformer extends Visitor {
     if (!(node instanceof BaseNode)) {
       return node;
     }
-    const nodename = node.type;
-    const visit = this[`visit_${nodename}`] || this.generic_visit;
+    const visit = this[`visit${node.type}`] || this.genericVisit;
     return visit.call(this, node);
   }
 
-  generic_visit(node) {
+  genericVisit(node) {
     for (const propname of Object.keys(node)) {
       const propvalue = node[propname];
       if (Array.isArray(propvalue)) {
diff --git a/fluent-syntax/test/ast_test.js b/fluent-syntax/test/ast_test.js
@@ -12,6 +12,7 @@ suite("BaseNode.equals", function() {
   test("Identifier.equals", function() {
     const thisNode = new AST.Identifier("name");
     const otherNode = new AST.Identifier("name");
+    assert.ok(thisNode.clone() instanceof AST.Identifier);
     assert.strictEqual(thisNode.equals(otherNode), true);
     assert.strictEqual(thisNode.equals(thisNode.clone()), true);
     assert.notStrictEqual(thisNode, thisNode.clone());
@@ -34,7 +35,7 @@ suite("BaseNode.equals", function() {
     ]);
     assert.strictEqual(thisNode.equals(otherNode), true);
   });
-  test("Variants", function() {
+  test("Variant order matters", function() {
       const thisRes = this.parser.parse(ftl`
           msg = { $val ->
               [few] things
@@ -43,7 +44,6 @@ suite("BaseNode.equals", function() {
             }
       `);
       const otherRes = this.parser.parse(ftl`
-          # a comment
           msg = { $val ->
               [few] things
              *[other] default
@@ -53,13 +53,10 @@ suite("BaseNode.equals", function() {
       const thisNode = thisRes.body[0];
       const otherNode = otherRes.body[0];
       assert.strictEqual(thisNode.equals(otherNode), false);
-      assert.strictEqual(thisNode.equals(otherNode, ['span', 'comment']), true);
-      assert.strictEqual(thisNode.value.equals(otherNode.value), true);
-      assert.strictEqual(thisNode.value.equals(otherNode.value, []), false);
       assert.strictEqual(thisRes.equals(thisRes.clone(), []), true);
       assert.notStrictEqual(thisRes, thisRes.clone());
   });
-  test("Attributes without order", function() {
+  test("Attribute order matters", function() {
       const thisRes = this.parser.parse(ftl`
           msg =
             .attr1 = one
@@ -72,6 +69,6 @@ suite("BaseNode.equals", function() {
       `);
       const thisNode = thisRes.body[0];
       const otherNode = otherRes.body[0];
-      assert.strictEqual(thisNode.equals(otherNode), true);
+      assert.strictEqual(thisNode.equals(otherNode), false);
   });
 });
diff --git a/fluent-syntax/test/visitor_test.js b/fluent-syntax/test/visitor_test.js
@@ -22,16 +22,16 @@ suite("Visitor", function() {
         this.calls = {};
         this.pattern_calls = 0;
       }
-      generic_visit(node) {
+      genericVisit(node) {
         const nodename = node.type;
         if (nodename in this.calls) {
           this.calls[nodename]++;
         } else {
           this.calls[nodename] = 1;
         }
-        super.generic_visit(node);
+        super.genericVisit(node);
       }
-      visit_Pattern(node) {
+      visitPattern(node) {
         this.pattern_calls++;
       }
     }
@@ -56,16 +56,16 @@ suite("Visitor", function() {
         super();
         this.word_count = 0;
       }
-      generic_visit(node) {
+      genericVisit(node) {
         switch (node.type) {
           case 'Span':
           case 'Annotation':            
             break;
           default:
-            super.generic_visit(node);
+            super.genericVisit(node);
         }
       }
-      visit_TextElement(node) {
+      visitTextElement(node) {
         this.word_count += node.value.split(/\s+/).length;
       }
     }
@@ -93,17 +93,17 @@ suite("Transformer", function() {
         this.before = before;
         this.after = after;
       }
-      generic_visit(node) {
+      genericVisit(node) {
         switch (node.type) {
           case 'Span':
           case 'Annotation':
             return node;
             break;
           default:
-            return super.generic_visit(node);
+            return super.genericVisit(node);
         }
       }
-      visit_TextElement(node) {
+      visitTextElement(node) {
         node.value = node.value.replace(this.before, this.after);
         return node;
       }

Original file line number	Diff line number	Diff line change
`@@ -33,19 +33,12 @@ export class BaseNode {`
`33`	`33`	`if (thisVal.length !== otherVal.length) {`
`34`	`34`	`return false;`
`35`	`35`	`}`
`36`		`- // Sort elements of order-agnostic fields to ensure the`
`37`		`- // comparison is order-agnostic as well. Annotations should be`
`38`		`- // here too but they don't have sorting keys.`
`39`		`- if (["attributes", "variants"].indexOf(fieldName) >= 0) {`
`40`		`- thisVal.sort(sorting_key_compare);`
`41`		`- otherVal.sort(sorting_key_compare);`
`42`		`- }`
`43`		`- for (let i = 0, ii = thisVal.length; i < ii; ++i) {`
`44`		`- if (!scalars_equal(thisVal[i], otherVal[i], ignoredFields)) {`
	`36`	`+ for (let i = 0; i < thisVal.length; ++i) {`
	`37`	`+ if (!scalarsEqual(thisVal[i], otherVal[i], ignoredFields)) {`
`45`	`38`	`return false;`
`46`	`39`	`}`
`47`	`40`	`}`
`48`		`- } else if (!scalars_equal(thisVal, otherVal, ignoredFields)) {`
	`41`	`+ } else if (!scalarsEqual(thisVal, otherVal, ignoredFields)) {`
`49`	`42`	`return false;`
`50`	`43`	`}`
`51`	`44`	`}`
`@@ -62,31 +55,21 @@ export class BaseNode {`
`62`	`55`	`}`
`63`	`56`	`return value;`
`64`	`57`	`}`
`65`		`- const clone = Object.create(this);`
	`58`	`+ const clone = Object.create(this.constructor.prototype);`
`66`	`59`	`for (const prop of Object.keys(this)) {`
`67`	`60`	`clone[prop] = visit(this[prop]);`
`68`	`61`	`}`
`69`	`62`	`return clone;`
`70`	`63`	`}`
`71`	`64`	`}`
`72`	`65`
`73`		`-function scalars_equal(thisVal, otherVal, ignoredFields) {`
	`66`	`+function scalarsEqual(thisVal, otherVal, ignoredFields) {`
`74`	`67`	`if (thisVal instanceof BaseNode) {`
`75`	`68`	`return thisVal.equals(otherVal, ignoredFields);`
`76`	`69`	`}`
`77`	`70`	`return thisVal === otherVal;`
`78`	`71`	`}`
`79`	`72`
`80`		`-function sorting_key_compare(left, right) {`
`81`		`- if (left.sorting_key < right.sorting_key) {`
`82`		`- return -1;`
`83`		`- }`
`84`		`- if (left.sorting_key === right.sorting_key) {`
`85`		`- return 0;`
`86`		`- }`
`87`		`- return 1;`
`88`		`-}`
`89`		`-`
`90`	`73`	`/*`
`91`	`74`	`* Base class for AST nodes which can have Spans.`
`92`	`75`	`*/`
`@@ -267,7 +250,7 @@ export class Attribute extends SyntaxNode {`
`267`	`250`	`this.value = value;`
`268`	`251`	`}`
`269`	`252`
`270`		`- get sorting_key() {`
	`253`	`+ get sortingKey() {`
`271`	`254`	`return this.id.name;`
`272`	`255`	`}`
`273`	`256`	`}`
`@@ -281,7 +264,7 @@ export class Variant extends SyntaxNode {`
`281`	`264`	`this.default = def;`
`282`	`265`	`}`
`283`	`266`
`284`		`- get sorting_key() {`
	`267`	`+ get sortingKey() {`
`285`	`268`	`if (this.key instanceof NumberLiteral) {`
`286`	`269`	`return this.key.value;`
`287`	`270`	`}`
Original file line number	Diff line number	Diff line change
`@@ -12,12 +12,11 @@ export class Visitor {`
`12`	`12`	`if (!(node instanceof BaseNode)) {`
`13`	`13`	`return;`
`14`	`14`	`}`
`15`		`- const nodename = node.type;`
`16`		- const visit = this[`visit_${nodename}`] \|\| this.generic_visit;
	`15`	+ const visit = this[`visit${node.type}`] \|\| this.genericVisit;
`17`	`16`	`visit.call(this, node);`
`18`	`17`	`}`
`19`	`18`
`20`		`- generic_visit(node) {`
	`19`	`+ genericVisit(node) {`
`21`	`20`	`for (const propname of Object.keys(node)) {`
`22`	`21`	`this.visit(node[propname]);`
`23`	`22`	`}`
`@@ -32,12 +31,11 @@ export class Transformer extends Visitor {`
`32`	`31`	`if (!(node instanceof BaseNode)) {`
`33`	`32`	`return node;`
`34`	`33`	`}`
`35`		`- const nodename = node.type;`
`36`		- const visit = this[`visit_${nodename}`] \|\| this.generic_visit;
	`34`	+ const visit = this[`visit${node.type}`] \|\| this.genericVisit;
`37`	`35`	`return visit.call(this, node);`
`38`	`36`	`}`
`39`	`37`
`40`		`- generic_visit(node) {`
	`38`	`+ genericVisit(node) {`
`41`	`39`	`for (const propname of Object.keys(node)) {`
`42`	`40`	`const propvalue = node[propname];`
`43`	`41`	`if (Array.isArray(propvalue)) {`
Original file line number	Diff line number	Diff line change
`@@ -22,16 +22,16 @@ suite("Visitor", function() {`
`22`	`22`	`this.calls = {};`
`23`	`23`	`this.pattern_calls = 0;`
`24`	`24`	`}`
`25`		`- generic_visit(node) {`
	`25`	`+ genericVisit(node) {`
`26`	`26`	`const nodename = node.type;`
`27`	`27`	`if (nodename in this.calls) {`
`28`	`28`	`this.calls[nodename]++;`
`29`	`29`	`} else {`
`30`	`30`	`this.calls[nodename] = 1;`
`31`	`31`	`}`
`32`		`- super.generic_visit(node);`
	`32`	`+ super.genericVisit(node);`
`33`	`33`	`}`
`34`		`- visit_Pattern(node) {`
	`34`	`+ visitPattern(node) {`
`35`	`35`	`this.pattern_calls++;`
`36`	`36`	`}`
`37`	`37`	`}`
`@@ -56,16 +56,16 @@ suite("Visitor", function() {`
`56`	`56`	`super();`
`57`	`57`	`this.word_count = 0;`
`58`	`58`	`}`
`59`		`- generic_visit(node) {`
	`59`	`+ genericVisit(node) {`
`60`	`60`	`switch (node.type) {`
`61`	`61`	`case 'Span':`
`62`	`62`	`case 'Annotation':`
`63`	`63`	`break;`
`64`	`64`	`default:`
`65`		`- super.generic_visit(node);`
	`65`	`+ super.genericVisit(node);`
`66`	`66`	`}`
`67`	`67`	`}`
`68`		`- visit_TextElement(node) {`
	`68`	`+ visitTextElement(node) {`
`69`	`69`	`this.word_count += node.value.split(/\s+/).length;`
`70`	`70`	`}`
`71`	`71`	`}`
`@@ -93,17 +93,17 @@ suite("Transformer", function() {`
`93`	`93`	`this.before = before;`
`94`	`94`	`this.after = after;`
`95`	`95`	`}`
`96`		`- generic_visit(node) {`
	`96`	`+ genericVisit(node) {`
`97`	`97`	`switch (node.type) {`
`98`	`98`	`case 'Span':`
`99`	`99`	`case 'Annotation':`
`100`	`100`	`return node;`
`101`	`101`	`break;`
`102`	`102`	`default:`
`103`		`- return super.generic_visit(node);`
	`103`	`+ return super.genericVisit(node);`
`104`	`104`	`}`
`105`	`105`	`}`
`106`		`- visit_TextElement(node) {`
	`106`	`+ visitTextElement(node) {`
`107`	`107`	`node.value = node.value.replace(this.before, this.after);`
`108`	`108`	`return node;`
`109`	`109`	`}`