From 5fd3055c97ef801d8a9a604e3e2ff178ffa5671b Mon Sep 17 00:00:00 2001 From: Joshua Goldberg Date: Sun, 14 Oct 2018 18:40:51 -0400 Subject: [PATCH] Added an InformativeDocsRule For any JSDoc comment on something with a name, if the comment only rephrases the name, this complains. Takes in configurable word aliases and a list of "useless" words that don't add to comments. I expect we'll slowly find more words to add to those defaults. Fixes #457. --- src/informativeDocsRule.ts | 179 ++++++++++++++++++++++++ src/tests/InformativeDocsRuleTests.ts | 192 ++++++++++++++++++++++++++ src/utils/NodeDocs.ts | 82 +++++++++++ 3 files changed, 453 insertions(+) create mode 100644 src/informativeDocsRule.ts create mode 100644 src/tests/InformativeDocsRuleTests.ts create mode 100644 src/utils/NodeDocs.ts diff --git a/src/informativeDocsRule.ts b/src/informativeDocsRule.ts new file mode 100644 index 000000000..42e005a5e --- /dev/null +++ b/src/informativeDocsRule.ts @@ -0,0 +1,179 @@ +import * as Lint from 'tslint'; +import * as ts from 'typescript'; + +import { getApparentJsDoc, getNodeName } from './utils/NodeDocs'; + +const defaultUselessWords = ['a', 'an', 'of', 'our', 'the']; + +const defaultAliases: { [i: string]: string[] } = { + a: ['an', 'our'], +}; + +const failureString = 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.'; + +interface RawOptions { + aliases?: { [i: string]: string[] }; + uselessWords?: string[]; +} + +interface Options { + aliases: Map; + uselessWords: Set; +} + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: Lint.IRuleMetadata = { + description: 'Enforces that comments do more than just reiterate names of objects.', + options: null, + optionsDescription: 'Not configurable.', + optionExamples: [ + true, + [true, { + aliases: { + a: ['an', 'our'], + emoji: ['smiley'] + }, + uselessWords: [...defaultUselessWords, 'also'] + }] + ], + rationale: Lint.Utils.dedent` + The documentation for an object should not be equivalent to just the object's name. + If we strip out non-alphabet characters, common words such as "the" or "a", + and lowercase everything, they shouldn't be the same. + + Using informative documentation can be helpful for variables to help explain their usage. + Alternately, if something's name is so descriptive that it doesn't need to be fully documented, + just leave out documentation altogether. + `, + ruleName: 'informative-docs', + type: 'maintainability', + typescriptOnly: false + }; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk, parseOptions(this.getOptions().ruleArguments)); + } +} + +function parseOptions(ruleArguments: any[]): Options { + const rawOptions: RawOptions = ruleArguments.length === 0 + ? {} + : ruleArguments[0]; + + return { + aliases: parseAliasesOption( + rawOptions.aliases === undefined + ? defaultAliases + : rawOptions.aliases + ), + uselessWords: new Set(rawOptions.uselessWords === undefined ? defaultUselessWords : rawOptions.uselessWords) + }; +} + +function parseAliasesOption(rawAliases: { [i: string]: string[] }): Map { + const aliases = new Map(); + + for (const alias of Object.keys(rawAliases)) { + for (const aliasedName of rawAliases[alias]) { + aliases.set(aliasedName.toLowerCase(), alias.toLowerCase()); + } + } + + return aliases; +} + +function walk(context: Lint.WalkContext) { + const { aliases, uselessWords } = context.options; + + function nodeNameContainsUsefulWords(nameWords: string[], docWords: string[]): boolean { + const realDocWords = new Set(docWords); + + for (const nameWord of nameWords) { + realDocWords.delete(nameWord); + } + + uselessWords.forEach((uselessWord: string): void => { + realDocWords.delete(uselessWord); + }); + + return realDocWords.size !== 0; + } + + function normalizeWord(word: string): string { + word = word.toLowerCase(); + + const aliasedWord = aliases.get(word); + if (aliasedWord !== undefined) { + word = aliasedWord; + } + + return word; + } + + function splitNameIntoWords(name: string): string[] | undefined { + if (name.length > 2 && name[0] === 'I' && Lint.Utils.isUpperCase(name[1])) { + name = name.substring(1); + } + + const nameSpaced = name + .replace(/\W/g, '') + .replace(/([a-z])([A-Z])/g, '$1 $2') + .trim(); + + if (nameSpaced.length === 0) { + return undefined; + } + + return nameSpaced + .split(' ') + .map(normalizeWord); + } + + function getNodeDocComments(node: ts.Node): string[] | undefined { + const docsRaw = getApparentJsDoc(node); + if (docsRaw === undefined) { + return undefined; + } + + const docs = docsRaw + .map((doc) => doc.comment) + .filter((comment) => comment !== undefined); + + if (docs.length === 0) { + return undefined; + } + + return docs + .join(' ') + .replace(/[^A-Za-z0-9 ]/g, '') + .split(' ') + .map(normalizeWord); + } + + function verifyNodeWithName(node: ts.Node, name: string): void { + const docs = getNodeDocComments(node); + if (docs === undefined) { + return; + } + + const nameSplit = splitNameIntoWords(name); + if (nameSplit === undefined) { + return; + } + + if (!nodeNameContainsUsefulWords(nameSplit, docs)) { + context.addFailureAtNode(node, failureString); + } + } + + function visitNode(node: ts.Node): void { + const name = getNodeName(node); + if (name !== undefined) { + verifyNodeWithName(node, name); + } + + return ts.forEachChild(node, visitNode); + } + + return ts.forEachChild(context.sourceFile, visitNode); +} diff --git a/src/tests/InformativeDocsRuleTests.ts b/src/tests/InformativeDocsRuleTests.ts new file mode 100644 index 000000000..bb411369f --- /dev/null +++ b/src/tests/InformativeDocsRuleTests.ts @@ -0,0 +1,192 @@ +import { TestHelper } from './TestHelper'; + +describe('informativeDocsRule', () : void => { + it('should pass on well-described functions', () : void => { + const script : string = ` + /** + * Does X Y Z work. + */ + function foo() {} + + /** + * Transforms the processed data. + */ + function bar() {} + + /** + * Transforms the processed baz data. + */ + function bazProcessor() {} + `; + + TestHelper.assertViolations('informative-docs', script, []); + }); + + it('should pass on a non-commented function', () : void => { + const script : string = ` + function foo() {} + `; + + TestHelper.assertViolations('informative-docs', script, []); + }); + + it('should fail on uninformative docs comments of all kinds', () : void => { + const script : string = ` + /** + * Foo class + */ + class FooClass { + /** + * the foo method declaration + */ + fooMethodDeclaration() {} + + /** + * The foo property declaration. + */ + public fooPropertyDeclaration; + } + + /** + * the Foo enum. + */ + enum FooEnum {} + + /** + * The foo function + */ + function fooFunction() {} + + const _ = { + /** + * The foo get accessor. + */ + get fooGetAccessor() { + return 0; + } + }; + + /** + * The foo interface. + */ + interface IFooInterface {} + + /** + * The foo module. + */ + module FooModule {} + + /** + * The foo type. + */ + type fooType = {}; + `; + + TestHelper.assertViolations('informative-docs', script, [ + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 5 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 17, line: 9 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 17, line: 14 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 20 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 25 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 17, line: 31 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 39 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 44 } + }, + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 49 } + } + ]); + }); + + it('should fail on uninformative docs comments using alias options', () : void => { + const script : string = ` + /** + * The FancyFoo. + */ + function foo() {} + `; + + const options = [ + { + aliases: { + foo: ['FancyFoo'] + } + } + ]; + + TestHelper.assertViolationsWithOptions('informative-docs', options, script, [ + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 5 } + } + ]); + }); + + it('should fail on uninformative docs comments using useless word options', () : void => { + const script : string = ` + /** + * Also foo. + */ + function foo() {} + `; + + const options = [ + { + uselessWords: ['also'] + } + ]; + + TestHelper.assertViolationsWithOptions('informative-docs', options, script, [ + { + failure: 'This comment is roughly the same as the object\'s name. Either be more informative or don\'t include a comment.', + name: 'file.ts', + ruleName: 'informative-docs', + startPosition: { character: 13, line: 5 } + } + ]); + }); +}); \ No newline at end of file diff --git a/src/utils/NodeDocs.ts b/src/utils/NodeDocs.ts new file mode 100644 index 000000000..edc8a251a --- /dev/null +++ b/src/utils/NodeDocs.ts @@ -0,0 +1,82 @@ +/*! + * Copyright Microsoft Corporation. All rights reserved. + */ + +import * as tsutils from 'tsutils'; +import * as ts from 'typescript'; + +/** + * Node types that may include a name identifier. + */ +type INodeWithNameIdentifier = ts.Node & { + name?: Partial; +}; + +/** + * Syntax kind identifiers for node types that may include a name identifier. + */ +const nodeKindsWithNameIdentifiers = new Set([ + ts.SyntaxKind.ClassDeclaration, + ts.SyntaxKind.EnumDeclaration, + ts.SyntaxKind.FunctionDeclaration, + ts.SyntaxKind.GetAccessor, + ts.SyntaxKind.InterfaceDeclaration, + ts.SyntaxKind.MethodDeclaration, + ts.SyntaxKind.ModuleDeclaration, + ts.SyntaxKind.PropertyDeclaration, + ts.SyntaxKind.SetAccessor, + ts.SyntaxKind.TypeAliasDeclaration, + ts.SyntaxKind.VariableStatement +]); + +/** + * @returns Name text of a node's name identifier, if it exists. + */ +const getNodeWithOptionalIdentifierName = (node: INodeWithNameIdentifier): string | undefined => { + const { name } = node; + + return name === undefined + ? undefined + : name.text; +}; + +/** + * @returns Whether this variable declaration is the first in its list. + */ +const variableIsAfterFirstInDeclarationList = (node: ts.VariableDeclaration): boolean => { + const parent = node.parent; + if (parent === undefined) { + return false; + } + + return ts.isVariableDeclarationList(parent) && node !== parent.declarations[0]; +}; + +/** + * @returns JSDoc that appears to be attached to a node, if one exists. + * @see https://github.com/ajafff/tsutils/issues/16 + */ +export const getApparentJsDoc = (node: ts.Node): ts.JSDoc[] | undefined => { + if (ts.isVariableDeclaration(node)) { + if (variableIsAfterFirstInDeclarationList(node)) { + return undefined; + } + + node = node.parent; + } + + if (ts.isVariableDeclarationList(node)) { + node = node.parent; + } + + return tsutils.getJsDoc(node); +}; + +/** + * @returns Name text of a node, if it contains a name identifier. + */ +export const getNodeName = (node: ts.Node): string | undefined => { + return nodeKindsWithNameIdentifiers.has(node.kind) + ? getNodeWithOptionalIdentifierName(node) + : undefined; +};