[doclint] simplify error reporting

This patch simplifies error reporting, making it straight-forward.
It also adds a few tests to make doclint more robust.

utils/doclint/Documentation.js

@@ -12,23 +12,18 @@ class Documentation {
   /**
    * @param {!Documentation} actual
    * @param {!Documentation} expected
+   * @return {!Array<string>}
    */
   static diff(actual, expected) {
-    // Diff classes.
-    const result = {
-      extraClasses: [],
-      missingClasses: [],
-      extraMethods: [],
-      missingMethods: [],
-      extraProperties: [],
-      missingProperties: [],
-      badArguments: [],
-    };
+    const errors = [];
+
     const actualClasses = Array.from(actual.classes.keys()).sort();
     const expectedClasses = Array.from(expected.classes.keys()).sort();
     let classesDiff = diff(actualClasses, expectedClasses);
-    result.extraClasses.push(...classesDiff.extra);
-    result.missingClasses.push(...classesDiff.missing);
+    for (let className of classesDiff.extra)
+      errors.push(`Non-existing class found: ${className}`);
+    for (let className of classesDiff.missing)
+      errors.push(`Class not found: ${className}`);
 
     for (let className of classesDiff.equal) {
       const actualClass = actual.classes.get(className);
@@ -36,29 +31,63 @@ class Documentation {
       const actualMethods = Array.from(actualClass.methods.keys()).sort();
       const expectedMethods = Array.from(expectedClass.methods.keys()).sort();
       const methodDiff = diff(actualMethods, expectedMethods);
-      result.extraMethods.push(...(methodDiff.extra.map(methodName => className + '.' + methodName)));
-      result.missingMethods.push(...(methodDiff.missing.map(methodName => className + '.' + methodName)));
+      for (let methodName of methodDiff.extra)
+        errors.push(`Non-existing method found: ${className}.${methodName}()`);
+      for (let methodName of methodDiff.missing)
+        errors.push(`Method not found: ${className}.${methodName}()`);
+
       for (let methodName of methodDiff.equal) {
         const actualMethod = actualClass.methods.get(methodName);
         const expectedMethod = expectedClass.methods.get(methodName);
-        const actualArgs = actualMethod.args.map(arg => arg.name);
-        const expectedArgs = expectedMethod.args.map(arg => arg.name);
+        const actualArgs = Array.from(actualMethod.args.keys());
+        const expectedArgs = Array.from(expectedMethod.args.keys());
         const argDiff = diff(actualArgs, expectedArgs);
         if (argDiff.extra.length || argDiff.missing.length) {
-          result.badArguments.push({
-            method: `${className}.${methodName}`,
-            missingArgs: argDiff.missing,
-            extraArgs: argDiff.extra
-          });
+          let text = [`Method ${className}.${methodName}() fails to describe its parameters:`];
+          for (let arg of argDiff.missing)
+            text.push(`- Argument not found: ${arg}`);
+          for (let arg of argDiff.extra)
+            text.push(`- Non-existing argument found: ${arg}`);
+          errors.push(text.join('\n'));
         }
       }
-      const actualProperties = actualClass.propertiesArray.slice().sort();
-      const expectedProperties = expectedClass.propertiesArray.slice().sort();
+      const actualProperties = Array.from(actualClass.properties.keys()).sort();
+      const expectedProperties = Array.from(expectedClass.properties.keys()).sort();
       const propertyDiff = diff(actualProperties, expectedProperties);
-      result.extraProperties.push(...(propertyDiff.extra.map(propertyName => className + '.' + propertyName)));
-      result.missingProperties.push(...(propertyDiff.missing.map(propertyName => className + '.' + propertyName)));
+      for (let propertyName of propertyDiff.extra)
+        errors.push(`Non-existing property found: ${className}.${propertyName}`);
+      for (let propertyName of propertyDiff.missing)
+        errors.push(`Property not found: ${className}.${propertyName}`);
     }
-    return result;
+    return errors;
+  }
+
+  /**
+   * @param {!Documentation} doc
+   * @return {!Array<string>}
+   */
+  static validate(doc) {
+    const errors = [];
+    let classes = new Set();
+    // Report duplicates.
+    for (let cls of doc.classesArray) {
+      if (classes.has(cls.name))
+        errors.push(`Duplicate declaration of class ${cls.name}`);
+      classes.add(cls.name);
+      let methods = new Set();
+      for (let method of cls.methodsArray) {
+        if (methods.has(method.name))
+          errors.push(`Duplicate declaration of method ${cls.name}.${method.name}()`);
+        methods.add(method.name);
+        let args = new Set();
+        for (let arg of method.argsArray) {
+          if (args.has(arg.name))
+            errors.push(`Duplicate declaration of argument ${cls.name}.${method.name} "${arg.name}"`);
+          args.add(arg.name);
+        }
+      }
+    }
+    return errors;
   }
 }
 
@@ -82,11 +111,14 @@ Documentation.Class = class {
 Documentation.Method = class {
   /**
    * @param {string} name
-   * @param {!Array<!Documentation.Argument>} args
+   * @param {!Array<!Documentation.Argument>} argsArray
    */
-  constructor(name, args) {
+  constructor(name, argsArray) {
     this.name = name;
-    this.args = args;
+    this.argsArray = argsArray;
+    this.args = new Map();
+    for (let arg of argsArray)
+      this.args.set(arg.name, arg);
   }
 };
 

utils/doclint/lint.js

@@ -39,79 +39,46 @@ async function lint(page, docsFolderPath, jsFolderPath) {
   let jsResult = await jsBuilder(jsFolderPath);
   let jsDocumentation = filterJSDocumentation(jsResult);
   let mdDocumentation = mdResult.documentation;
-  let diff = Documentation.diff(mdDocumentation, jsDocumentation);
 
   let jsErrors = [];
-  let mdErrors = [];
-
+  let mdErrors = Documentation.diff(mdDocumentation, jsDocumentation);
   // Report all markdown parse errors.
   mdErrors.push(...mdResult.errors);
-  {
-    // Report duplicate JavaScript classes.
-    let jsClasses = new Map();
-    for (let jsClass of jsDocumentation.classesArray) {
-      if (jsClasses.has(jsClass.name))
-        jsErrors.push(`Duplicate declaration of class ${jsClass.name}`);
-      jsClasses.set(jsClass.name, jsClass);
-    }
-  }
-  {
-    // Report duplicate MarkDown classes.
-    let mdClasses = new Map();
-    for (let mdClass of mdDocumentation.classesArray) {
-      if (mdClasses.has(mdClass.name))
-        mdErrors.push(`Duplicate declaration of class ${mdClass.name}`);
-      mdClasses.set(mdClass.name, mdClass);
-    }
-  }
-  {
-    // Make sure class constructors are defined before other methods.
-    for (let mdClass of mdDocumentation.classesArray) {
-      let constructorMethod = mdClass.methods.get('constructor');
-      if (!constructorMethod)
-        continue;
-      if (mdClass.methodsArray[0] !== constructorMethod)
-        mdErrors.push(`Constructor of ${mdClass.name} should go before other methods`);
-    }
-  }
-  {
-    // Methods should be sorted alphabetically.
-    for (let mdClass of mdDocumentation.classesArray) {
-      for (let i = 0; i < mdClass.methodsArray.length - 1; ++i) {
-        // Constructor should always go first.
-        if (mdClass.methodsArray[i].name === 'constructor')
-          continue;
-        let method1 = mdClass.methodsArray[i];
-        let method2 = mdClass.methodsArray[i + 1];
-        if (method1.name > method2.name)
-          mdErrors.push(`${mdClass.name}.${method1.name} breaks alphabetic sorting inside class ${mdClass.name}`);
-      }
-    }
-  }
-  // Report non-existing and missing classes.
-  mdErrors.push(...diff.extraClasses.map(className => `Non-existing class found: ${className}`));
-  mdErrors.push(...diff.missingClasses.map(className => `Class not found: ${className}`));
-  mdErrors.push(...diff.extraMethods.map(methodName => `Non-existing method found: ${methodName}`));
-  mdErrors.push(...diff.missingMethods.map(methodName => `Method not found: ${methodName}`));
-  mdErrors.push(...diff.extraProperties.map(propertyName => `Non-existing property found: ${propertyName}`));
-  mdErrors.push(...diff.missingProperties.map(propertyName => `Property not found: ${propertyName}`));
-  {
-    // Report badly described arguments.
-    for (let badArgument of diff.badArguments) {
-      let text = [`Method ${badArgument.method} fails to describe its parameters:`];
-      for (let missing of badArgument.missingArgs)
-        text.push(`- Missing description for "${missing}"`);
-      for (let extra of badArgument.extraArgs)
-        text.push(`- Described non-existing parameter "${extra}"`);
-      mdErrors.push(text.join('\n'));
-    }
-  }
+
+  jsErrors.push(...Documentation.validate(jsDocumentation));
+  mdErrors.push(...Documentation.validate(mdDocumentation));
+  mdErrors.push(...lintMarkdown(mdDocumentation));
+
   // Push all errors with proper prefixes
   let errors = jsErrors.map(error => '[JavaScript] ' + error);
   errors.push(...mdErrors.map(error => '[MarkDown] ' + error));
   return errors;
 }
 
+/**
+ * @param {!Documentation} doc
+ * @return {!Array<string>}
+ */
+function lintMarkdown(doc) {
+  const errors = [];
+  // Methods should be sorted alphabetically.
+  for (let cls of doc.classesArray) {
+    for (let i = 0; i < cls.methodsArray.length - 1; ++i) {
+      // Constructor always goes first.
+      if (cls.methodsArray[i].name === 'constructor') {
+        if (i > 0)
+          errors.push(`Constructor of ${cls.name} should go before other methods`);
+        continue;
+      }
+      let method1 = cls.methodsArray[i];
+      let method2 = cls.methodsArray[i + 1];
+      if (method1.name > method2.name)
+        errors.push(`${cls.name}.${method1.name} breaks alphabetic methods sorting inside class ${cls.name}`);
+    }
+  }
+  return errors;
+}
+
 /**
  * @param {!Documentation} jsDocumentation
  * @return {!Documentation}

utils/doclint/test/06-duplicates/doc.md

@@ -0,0 +1,14 @@
+### class: Bar
+
+### class: Foo
+
+#### foo.test()
+
+#### foo.test()
+
+#### foo.title(arg, arg)
+- `arg` <[number]>
+- `arg` <[number]>
+
+### class: Bar
+

utils/doclint/test/06-duplicates/foo.js

@@ -0,0 +1,10 @@
+class Foo {
+  test() {
+  }
+
+  title(arg) {
+  }
+}
+
+class Bar {
+}

utils/doclint/test/07-sorting/doc.md

@@ -0,0 +1,9 @@
+### class: Foo
+
+#### foo.aaa()
+
+#### new Foo()
+
+#### foo.ccc()
+
+#### foo.bbb()

utils/doclint/test/07-sorting/foo.js

@@ -0,0 +1,10 @@
+class Foo {
+  constructor() {
+  }
+
+  aaa() {}
+
+  bbb() {}
+
+  ccc() {}
+}

utils/doclint/test/golden/02-method-errors.txt

@@ -1,2 +1,2 @@
-[MarkDown] Non-existing method found: Foo.proceed
-[MarkDown] Method not found: Foo.stop
+[MarkDown] Non-existing method found: Foo.proceed()
+[MarkDown] Method not found: Foo.stop()

utils/doclint/test/golden/03-property-errors.txt

@@ -1,3 +1,3 @@
-[MarkDown] Method not found: Foo.constructor
+[MarkDown] Method not found: Foo.constructor()
 [MarkDown] Non-existing property found: Foo.c
 [MarkDown] Property not found: Foo.b

utils/doclint/test/golden/04-bad-arguments.txt

@@ -1,4 +1,4 @@
+[MarkDown] Method Foo.constructor() fails to describe its parameters:
+- Argument not found: arg3
+- Non-existing argument found: arg2
 [MarkDown] Heading arguments for "foo.test(fileNames)" do not match described ones, i.e. "fileNames" != "filePaths"
-[MarkDown] Method Foo.constructor fails to describe its parameters:
-- Missing description for "arg3"
-- Described non-existing parameter "arg2"

utils/doclint/test/golden/06-duplicates.txt

@@ -0,0 +1,3 @@
+[MarkDown] Duplicate declaration of method Foo.test()
+[MarkDown] Duplicate declaration of argument Foo.title "arg"
+[MarkDown] Duplicate declaration of class Bar

utils/doclint/test/golden/07-sorting.txt

@@ -0,0 +1,2 @@
+[MarkDown] Constructor of Foo should go before other methods
+[MarkDown] Foo.ccc breaks alphabetic methods sorting inside class Foo

utils/doclint/test/test.js

@@ -36,6 +36,8 @@ describe('doclint', function() {
   it('03-property-errors', SX(test));
   it('04-bad-arguments', SX(test));
   it('05-outdated-toc', SX(test));
+  it('06-duplicates', SX(test));
+  it('07-sorting', SX(test));
 });
 
 async function test() {