From 56619baa6458af544efd769ea4ddb0144bdb5ab8 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 13 Jul 2017 15:15:31 -0700 Subject: [PATCH] [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 | 90 ++++++++++++------ utils/doclint/lint.js | 93 ++++++------------- utils/doclint/test/06-duplicates/doc.md | 14 +++ utils/doclint/test/06-duplicates/foo.js | 10 ++ utils/doclint/test/07-sorting/doc.md | 9 ++ utils/doclint/test/07-sorting/foo.js | 10 ++ .../doclint/test/golden/02-method-errors.txt | 4 +- .../test/golden/03-property-errors.txt | 2 +- .../doclint/test/golden/04-bad-arguments.txt | 8 +- utils/doclint/test/golden/06-duplicates.txt | 3 + utils/doclint/test/golden/07-sorting.txt | 2 + utils/doclint/test/test.js | 2 + 12 files changed, 148 insertions(+), 99 deletions(-) create mode 100644 utils/doclint/test/06-duplicates/doc.md create mode 100644 utils/doclint/test/06-duplicates/foo.js create mode 100644 utils/doclint/test/07-sorting/doc.md create mode 100644 utils/doclint/test/07-sorting/foo.js create mode 100644 utils/doclint/test/golden/06-duplicates.txt create mode 100644 utils/doclint/test/golden/07-sorting.txt diff --git a/utils/doclint/Documentation.js b/utils/doclint/Documentation.js index 41797b5b3b8..1fe1b61f812 100644 --- a/utils/doclint/Documentation.js +++ b/utils/doclint/Documentation.js @@ -12,23 +12,18 @@ class Documentation { /** * @param {!Documentation} actual * @param {!Documentation} expected + * @return {!Array} */ 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} + */ + 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} args + * @param {!Array} 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); } }; diff --git a/utils/doclint/lint.js b/utils/doclint/lint.js index 66108994e47..012315deb88 100644 --- a/utils/doclint/lint.js +++ b/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} + */ +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} diff --git a/utils/doclint/test/06-duplicates/doc.md b/utils/doclint/test/06-duplicates/doc.md new file mode 100644 index 00000000000..1e3b621c618 --- /dev/null +++ b/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 + diff --git a/utils/doclint/test/06-duplicates/foo.js b/utils/doclint/test/06-duplicates/foo.js new file mode 100644 index 00000000000..b8dcec5dc4c --- /dev/null +++ b/utils/doclint/test/06-duplicates/foo.js @@ -0,0 +1,10 @@ +class Foo { + test() { + } + + title(arg) { + } +} + +class Bar { +} diff --git a/utils/doclint/test/07-sorting/doc.md b/utils/doclint/test/07-sorting/doc.md new file mode 100644 index 00000000000..5e9c03e7416 --- /dev/null +++ b/utils/doclint/test/07-sorting/doc.md @@ -0,0 +1,9 @@ +### class: Foo + +#### foo.aaa() + +#### new Foo() + +#### foo.ccc() + +#### foo.bbb() diff --git a/utils/doclint/test/07-sorting/foo.js b/utils/doclint/test/07-sorting/foo.js new file mode 100644 index 00000000000..b904f3c5544 --- /dev/null +++ b/utils/doclint/test/07-sorting/foo.js @@ -0,0 +1,10 @@ +class Foo { + constructor() { + } + + aaa() {} + + bbb() {} + + ccc() {} +} diff --git a/utils/doclint/test/golden/02-method-errors.txt b/utils/doclint/test/golden/02-method-errors.txt index 0c13eb517b7..6295473cf17 100644 --- a/utils/doclint/test/golden/02-method-errors.txt +++ b/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 \ No newline at end of file +[MarkDown] Non-existing method found: Foo.proceed() +[MarkDown] Method not found: Foo.stop() \ No newline at end of file diff --git a/utils/doclint/test/golden/03-property-errors.txt b/utils/doclint/test/golden/03-property-errors.txt index aa50ead70e1..785e9904b76 100644 --- a/utils/doclint/test/golden/03-property-errors.txt +++ b/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 \ No newline at end of file diff --git a/utils/doclint/test/golden/04-bad-arguments.txt b/utils/doclint/test/golden/04-bad-arguments.txt index 813256c1a8d..2b03dbcea04 100644 --- a/utils/doclint/test/golden/04-bad-arguments.txt +++ b/utils/doclint/test/golden/04-bad-arguments.txt @@ -1,4 +1,4 @@ -[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" \ No newline at end of file +[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" \ No newline at end of file diff --git a/utils/doclint/test/golden/06-duplicates.txt b/utils/doclint/test/golden/06-duplicates.txt new file mode 100644 index 00000000000..d9994416e91 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/utils/doclint/test/golden/07-sorting.txt b/utils/doclint/test/golden/07-sorting.txt new file mode 100644 index 00000000000..567dc2c3f55 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/utils/doclint/test/test.js b/utils/doclint/test/test.js index 035bd3c1de9..3f21ff6b554 100644 --- a/utils/doclint/test/test.js +++ b/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() {