refactor(nest): Better nesting implementation

tmcw · tmcw · commit 921874b19148 · 2017-04-15T11:13:49.000-04:00
This nesting implementation uses a proper recursive tree algorithm Fixes #554
diff --git a/declarations/comment.js b/declarations/comment.js
@@ -58,7 +58,21 @@ declare type CommentContextGitHub = {
   url: string
 };
 
-declare type CommentTag = {
+declare type CommentTagBase = {
+  title: string
+};
+
+declare type CommentTag = CommentTagBase & {
+  name?: string,
+  title: string,
+  description?: Object,
+  default?: any,
+  lineNumber?: number,
+  type?: DoctrineType,
+  properties?: Array<CommentTag>
+};
+
+declare type CommentTagNamed = CommentTag & {
   name?: string,
   title: string,
   description?: Object,
@@ -88,18 +102,19 @@ declare type Remark = {
 
 declare type Access = 'private' | 'public' | 'protected';
 declare type Scope = 'instance' | 'static' | 'inner' | 'global';
-declare type Kind = 'class' |
-  'constant' |
-  'event' |
-  'external' |
-  'file' |
-  'function' |
-  'member' |
-  'mixin' |
-  'module' |
-  'namespace' |
-  'typedef' |
-  'interface';
+declare type Kind =
+  | 'class'
+  | 'constant'
+  | 'event'
+  | 'external'
+  | 'file'
+  | 'function'
+  | 'member'
+  | 'mixin'
+  | 'module'
+  | 'namespace'
+  | 'typedef'
+  | 'interface';
 
 declare type Comment = {
   errors: Array<CommentError>,
@@ -152,4 +167,4 @@ declare type ReducedComment = {
   name: string,
   kind: ?Kind,
   scope?: ?Scope
-}
+};
diff --git a/lib/nest.js b/lib/nest.js
@@ -1,62 +1,78 @@
 /* @flow */
 'use strict';
 
+var _ = require('lodash');
+
+const PATH_SPLIT = /(?:\[])?\./g;
+
+function rejectUnnamedTags(
+  tags /*: Array<CommentTag> */
+) /*: Array<CommentTagNamed> */ {
+  return tags.filter(tag => typeof tag.name === 'string');
+}
+
 /**
  * Nest nestable tags, like param and property, into nested
  * arrays that are suitable for output.
+ *Okay, so we're building a tree of comments, with the tagName
+ * being the indexer
+ *
+ * foo.abe
+ * foo.bar.baz
+ * foo.bar.a
+ * foo.bar[].bax
+ *
+ * foo -> .abe
+ *    \-> .bar -> .baz
+ *            \-> .a
+ *            \-> [].baz
  *
  * @private
  * @param {Object} comment a comment with tags
  * @param {string} tagTitle the tag to nest
  * @param {string} target the tag to nest into
  * @returns {Object} nested comment
  */
-function nestTag(
-  comment /*: Comment */,
-  tagTitle /*: string */,
-  target /*: string */
-) {
-  if (!comment[target]) {
-    return comment;
-  }
+var nestTag = (
+  tags /*: Array<CommentTag> */
+  // Use lodash here both for brevity and also because, unlike JavaScript's
+  // sort, it's stable.
+) =>
+  _.sortBy(
+    rejectUnnamedTags(tags),
+    tag => tag.name.split(PATH_SPLIT).length
+  ).reduce(
+    (memo, tag) => {
+      function insertTag(node, parts) {
+        // The 'done' case: we're at parts.length === 1,
+        // this is where the node should be placed. We reliably
+        // get to this case because the recursive method
+        // is always passed parts.slice(1)
+        if (parts.length === 1) {
+          _.assign(node, {
+            properties: (node.properties || []).concat(tag)
+          });
+        } else {
+          // The recursive case: try to find the child that owns
+          // this tag.
+          let child = node.properties &&
+            node.properties.find(
+              property => parts[0] === _.last(property.name.split(PATH_SPLIT))
+            );
 
-  var result = [], index = {};
+          if (!child) {
+            throw new Error(`Parent of ${tag.name} not found `);
+          }
 
-  comment[target].forEach(tag => {
-    var tagName = tag.name;
-    if (tagName) {
-      index[tagName] = tag;
-      var parts = tagName
-        .split(/(\[\])?\./)
-        .filter(part => part && part !== '[]');
-      if (parts.length > 1) {
-        var parent = index[parts.slice(0, -1).join('.')];
-        if (parent === undefined) {
-          comment.errors.push({
-            message: '@' +
-              tagTitle +
-              ' ' +
-              tag.name +
-              "'s parent " +
-              parts[0] +
-              ' not found',
-            commentLineNumber: tag.lineNumber
-          });
-          result.push(tag);
-          return;
+          insertTag(child, parts.slice(1));
         }
-        parent.properties = parent.properties || [];
-        parent.properties.push(tag);
-      } else {
-        result.push(tag);
       }
-    }
-  });
-
-  comment[target] = result;
 
-  return comment;
-}
+      insertTag(memo, tag.name.split(PATH_SPLIT));
+      return memo;
+    },
+    { properties: [] }
+  ).properties;
 
 /**
  * Nests
@@ -70,8 +86,11 @@ function nestTag(
  * @param {Object} comment input comment
  * @return {Object} nested comment
  */
-function nest(comment /*: Comment*/) {
-  return nestTag(nestTag(comment, 'param', 'params'), 'property', 'properties');
-}
+var nest = (comment /*: Comment*/) =>
+  _.assign(comment, {
+    params: nestTag(comment.params),
+    properties: nestTag(comment.properties)
+  });
 
 module.exports = nest;
+module.exports.nestTag = nestTag;
diff --git a/test/lib/nest.js b/test/lib/nest.js
@@ -1,101 +1,117 @@
 'use strict';
 
-var test = require('tap').test,
-  parse = require('../../lib/parsers/javascript'),
-  nest = require('../../lib/nest');
+var test = require('tap').test;
+var nestTag = require('../../lib/nest').nestTag;
 
-function toComment(fn, filename) {
-  return parse(
-    {
-      file: filename,
-      source: fn instanceof Function ? '(' + fn.toString() + ')' : fn
-    },
-    {}
-  ).map(nest);
-}
+// Print a tree of tags in a way that's easy to test.
+var printTree = indent =>
+  node =>
+    `${new Array(indent + 1).join(' ')}- ${node.name}${node.properties ? '\n' : ''}${(node.properties || [
+    ])
+      .map(printTree(indent + 1))
+      .join('\n')}`;
 
-test('nest params - no params', function(t) {
-  t.deepEqual(
-    toComment(function() {
-      /** @name foo */
-    })[0].params,
-    [],
-    'no params'
-  );
-  t.end();
-});
+var printNesting = params =>
+  printTree(0)({ properties: nestTag(params), name: 'root' });
 
-test('nest params - no nesting', function(t) {
-  var result = toComment(function() {
-    /** @param {Object} foo */
-  });
-  t.equal(result[0].params.length, 1);
-  t.equal(result[0].params[0].name, 'foo');
-  t.equal(result[0].params[0].properties, undefined);
+test('nest params - basic', function(t) {
+  var params = [
+    'foo',
+    'foo.bar',
+    'foo.bar.third',
+    'foo.third',
+    'foo.third[].baz'
+  ].map(name => ({ name }));
+  t.equal(
+    printNesting(params),
+    `- root
+ - foo
+  - foo.bar
+   - foo.bar.third
+  - foo.third
+   - foo.third[].baz`
+  );
   t.end();
 });
 
-test('nest params - basic', function(t) {
-  var result = toComment(function() {
-    /**
-     * @param {Object} foo
-     * @param {string} foo.bar
-     * @param {string} foo.baz
-     */
-  });
-  t.equal(result[0].params.length, 1);
-  t.equal(result[0].params[0].name, 'foo');
-  t.equal(result[0].params[0].properties.length, 2);
-  t.equal(result[0].params[0].properties[0].name, 'foo.bar');
-  t.equal(result[0].params[0].properties[1].name, 'foo.baz');
+test('nest params - multiple roots', function(t) {
+  var params = ['a', 'b', 'c'].map(name => ({ name }));
+  t.equal(
+    printNesting(params),
+    `- root
+ - a
+ - b
+ - c`
+  );
   t.end();
 });
 
-test('nest properties - basic', function(t) {
-  var result = toComment(function() {
-    /**
-     * @property {Object} foo
-     * @property {string} foo.bar
-     * @property {string} foo.baz
-     */
+test('nest params - missing parent', function(t) {
+  var params = ['foo', 'foo.bar.third'].map(name => ({ name }));
+  t.throws(() => {
+    nestTag(params);
   });
-  t.equal(result[0].properties.length, 1);
-  t.equal(result[0].properties[0].name, 'foo');
-  t.equal(result[0].properties[0].properties.length, 2);
-  t.equal(result[0].properties[0].properties[0].name, 'foo.bar');
-  t.equal(result[0].properties[0].properties[1].name, 'foo.baz');
   t.end();
 });
 
-test('nest params - array', function(t) {
-  var result = toComment(function() {
-    /**
-     * @param {Object[]} employees - The employees who are responsible for the project.
-     * @param {string} employees[].name - The name of an employee.
-     * @param {string} employees[].department - The employee's department.
-     */
-  });
-  t.equal(result[0].params.length, 1);
-  t.equal(result[0].params[0].name, 'employees');
-  t.equal(result[0].params[0].properties.length, 2);
-  t.equal(result[0].params[0].properties[0].name, 'employees[].name');
-  t.equal(result[0].params[0].properties[1].name, 'employees[].department');
+test('nest params - #658', function(t) {
+  var params = [
+    'state',
+    'payload',
+    'payload.input_meter_levels',
+    'payload.input_meter_levels[].peak',
+    'payload.input_meter_levels[].rms',
+    'payload.output_meter_levels',
+    'payload.output_meter_levels[].peak',
+    'payload.output_meter_levels[].rms'
+  ].map(name => ({ name }));
+  t.equal(
+    printNesting(params),
+    `- root
+ - state
+ - payload
+  - payload.input_meter_levels
+   - payload.input_meter_levels[].peak
+   - payload.input_meter_levels[].rms
+  - payload.output_meter_levels
+   - payload.output_meter_levels[].peak
+   - payload.output_meter_levels[].rms`
+  );
   t.end();
 });
 
-test('nest params - missing parent', function(t) {
-  var result = toComment(function() {
-    /** @param {string} foo.bar */
-  });
-  t.equal(result[0].params.length, 1);
-  t.deepEqual(
-    result[0].errors[0],
-    {
-      message: "@param foo.bar's parent foo not found",
-      commentLineNumber: 0
-    },
-    'correct error message'
+test('nest params - #554', function(t) {
+  var params = [
+    'x',
+    'yIn',
+    'options',
+    'options.sgOption',
+    'options.minMaxRatio',
+    'options.broadRatio',
+    'options.noiseLevel',
+    'options.maxCriteria',
+    'options.smoothY',
+    'options.realTopDetection',
+    'options.heightFactor',
+    'options.boundaries',
+    'options.derivativeThreshold'
+  ].map(name => ({ name }));
+  t.equal(
+    printNesting(params),
+    `- root
+ - x
+ - yIn
+ - options
+  - options.sgOption
+  - options.minMaxRatio
+  - options.broadRatio
+  - options.noiseLevel
+  - options.maxCriteria
+  - options.smoothY
+  - options.realTopDetection
+  - options.heightFactor
+  - options.boundaries
+  - options.derivativeThreshold`
   );
-  t.equal(result[0].params[0].name, 'foo.bar');
   t.end();
 });
