Review comments. Mostly camelCases. And Object.create

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.

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,26 +55,26 @@ 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) {
+function sortingKeyCompare(left, right) {
+  if (left.sortingKey < right.sortingKey) {
     return -1;
   }
-  if (left.sorting_key === right.sorting_key) {
+  if (left.sortingKey === right.sortingKey) {
     return 0;
   }
   return 1;
@@ -267,7 +260,7 @@ export class Attribute extends SyntaxNode {
     this.value = value;
   }
 
-  get sorting_key() {
+  get sortingKey() {
     return this.id.name;
   }
 }
@@ -281,7 +274,7 @@ export class Variant extends SyntaxNode {
     this.default = def;
   }
 
-  get sorting_key() {
+  get sortingKey() {
     if (this.key instanceof NumberLiteral) {
       return this.key.value;
     }

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)) {

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);
   });
 });

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;
       }