Review comments. Mostly camelCases. And Object.create

Added an explicit test to ensure that SomeNode.clone creates the same class.
projectfluent · Feb 27, 2019 · 8f98965 · 8f98965
1 parent 387bc7e
commit 8f98965
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 24 deletions.
diff --git a/fluent-syntax/src/ast.js b/fluent-syntax/src/ast.js
@@ -36,16 +36,16 @@ export class BaseNode {
         // 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);
+        if (["attributes", "variants"].includes(fieldName)) {
+          thisVal.sort(sortingKeyCompare);
+          otherVal.sort(sortingKeyCompare);
         }
         for (let i = 0, ii = thisVal.length; i < ii; ++i) {
-          if (!scalars_equal(thisVal[i], otherVal[i], ignoredFields)) {
+          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 +62,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 +267,7 @@ export class Attribute extends SyntaxNode {
     this.value = value;
   }
 
-  get sorting_key() {
+  get sortingKey() {
     return this.id.name;
   }
 }
@@ -281,7 +281,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());

diff --git a/fluent-syntax/test/visitor_test.js b/fluent-syntax/test/visitor_test.js
@@ -22,14 +22,14 @@ 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) {
         this.pattern_calls++;
@@ -56,13 +56,13 @@ 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) {
@@ -93,14 +93,14 @@ 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) {