[doclint] Start linting method arguments

This patch introduces a general Documentation.diff method, which
produces a diff of two documentations.

With this, the patch teaches documentation linter to lint method arguments.

References #14.
This commit is contained in:
Andrey Lushnikov 2017-07-11 08:57:26 -07:00
parent f05cc3168c
commit 7765760186
7 changed files with 372 additions and 173 deletions

View File

@ -14,8 +14,8 @@
* [page.addScriptTag(url)](#pageaddscripttagurl)
* [page.click()](#pageclick)
* [page.close()](#pageclose)
* [page.evaluate(fun, args)](#pageevaluatefun-args)
* [page.evaluateOnInitialized(fun, args)](#pageevaluateoninitializedfun-args)
* [page.evaluate(fun, ...args)](#pageevaluatefun-args)
* [page.evaluateOnInitialized(fun, ...args)](#pageevaluateoninitializedfun-args)
* [page.focus()](#pagefocus)
* [page.frames()](#pageframes)
* [page.httpHeaders()](#pagehttpheaders)
@ -44,7 +44,7 @@
* [dialog.message()](#dialogmessage)
- [class: Frame](#class-frame)
* [frame.childFrames()](#framechildframes)
* [frame.evaluate(fun, args)](#frameevaluatefun-args)
* [frame.evaluate(fun, ...args)](#frameevaluatefun-args)
* [frame.isDetached()](#frameisdetached)
* [frame.isMainFrame()](#frameismainframe)
* [frame.name()](#framename)
@ -133,16 +133,16 @@ Pages could be closed by `page.close()` method.
- returns: <[Promise]> Returns promise which resolves when page gets closed.
#### page.evaluate(fun, args)
#### page.evaluate(fun, ...args)
- `fun` <[function]> Function to be evaluated in browser context
- `args` <[Array]<[string]>> Arguments to pass to `fun`
- `...args` <...[string]> Arguments to pass to `fun`
- returns: <[Promise]<[Object]>> Promise which resolves to function return value
#### page.evaluateOnInitialized(fun, args)
#### page.evaluateOnInitialized(fun, ...args)
- `fun` <[function]> Function to be evaluated in browser context
- `args` <[Array]<[string]>> Arguments to pass to `fun`
- `...args` <...[string]> Arguments to pass to `fun`
- returns: <[Promise]<[Object]>> Promise which resolves to function
#### page.focus()
@ -156,7 +156,7 @@ Pages could be closed by `page.close()` method.
#### page.injectFile(filePath)
- `url` <[string]> Path to the javascript file to be injected into page.
- `filePath` <[string]> Path to the javascript file to be injected into page.
- returns: <[Promise]> Promise which resolves when file gets successfully evaluated in page.
#### page.mainFrame()
@ -222,7 +222,7 @@ The `page.navigate` will throw an error if:
#### page.setInPageCallback(name, callback)
- `url` <[string]> Name of the callback to be assigned on window object
- `name` <[string]> Name of the callback to be assigned on window object
- `callback` <[function]> Callback function which will be called in node.js
- returns: <[Promise]> Promise which resolves when callback is successfully initialized
@ -267,6 +267,12 @@ The `page.navigate` will throw an error if:
#### page.waitFor(selector)
- `selector` <[string]> A query selector to wait for on the page.
Wait for the `selector` to appear in page. If at the moment of calling
the method the `selector` already exists, the method will return
immediately.
Shortcut for [page.mainFrame().waitFor(selector)](#framewaitforselector).
### class: Dialog
@ -276,10 +282,10 @@ Shortcut for [page.mainFrame().waitFor(selector)](#framewaitforselector).
### class: Frame
#### frame.childFrames()
#### frame.evaluate(fun, args)
#### frame.evaluate(fun, ...args)
- `fun` <[function]> Function to be evaluated in browser context
- `args` <[Array]<[string]>> Arguments to pass to `fun`
- `...args` <[Array]<[string]>> Arguments to pass to `fun`
- returns: <[Promise]<[Object]>> Promise which resolves to function return value
#### frame.isDetached()

View File

@ -583,13 +583,13 @@ class Page extends EventEmitter {
/**
* @param {string} selector
* @param {!Array<string>} files
* @param {!Array<string>} filePaths
* @return {!Promise}
*/
async uploadFile(selector, ...files) {
async uploadFile(selector, ...filePaths) {
try {
const nodeId = await this._querySelector(selector);
await this._client.send('DOM.setFileInputFiles', { nodeId, files });
await this._client.send('DOM.setFileInputFiles', { nodeId, files: filePaths });
} finally {
await this._client.send('DOM.disable');
}

View File

@ -1,4 +1,56 @@
let Documentation = {};
class Documentation {
/**
* @param {!Array<!Documentation.Class>} clasesArray
*/
constructor(classesArray) {
this.classesArray = classesArray;
this.classes = new Map();
for (let cls of classesArray)
this.classes.set(cls.name, cls);
}
/**
* @param {!Documentation} actual
* @param {!Documentation} expected
*/
static diff(actual, expected) {
const result = {};
// Diff classes.
const actualClasses = Array.from(actual.classes.keys()).sort();
const expectedClasses = Array.from(expected.classes.keys()).sort();
let classesDiff = diff(actualClasses, expectedClasses);
result.extraClasses = classesDiff.extra;
result.missingClasses = classesDiff.missing;
result.extraMethods = [];
result.missingMethods = [];
result.badArguments = [];
for (let className of classesDiff.equal) {
const actualClass = actual.classes.get(className);
const expectedClass = expected.classes.get(className);
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.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 argDiff = diff(actualArgs, expectedArgs);
if (argDiff.extra.length || argDiff.missing.diff) {
result.badArguments.push({
method: `${className}.${methodName}`,
missingArgs: argDiff.missing,
extraArgs: argDiff.extra
});
}
}
}
return result;
}
}
Documentation.Class = class {
/**
@ -15,6 +67,17 @@ Documentation.Class = class {
};
Documentation.Method = class {
/**
* @param {string} name
* @param {!Array<!Documentation.Argument>} args
*/
constructor(name, args) {
this.name = name;
this.args = args;
}
};
Documentation.Argument = class {
/**
* @param {string} name
*/
@ -23,4 +86,78 @@ Documentation.Method = class {
}
};
/**
* @param {!Array<string>} actual
* @param {!Array<string>} expected
* @return {{extra: !Array<string>, missing: !Array<string>, equal: !Array<string>}}
*/
function diff(actual, expected) {
const N = actual.length;
const M = expected.length;
if (N === 0 && M === 0)
return { extra: [], missing: [], equal: []};
if (N === 0)
return {extra: [], missing: expected.slice(), equal: []};
if (M === 0)
return {extra: actual.slice(), missing: [], equal: []};
let d = new Array(N);
let bt = new Array(N);
for (let i = 0; i < N; ++i) {
d[i] = new Array(M);
bt[i] = new Array(M);
for (let j = 0; j < M; ++j) {
const top = val(i - 1, j);
const left = val(i, j - 1);
if (top > left) {
d[i][j] = top;
bt[i][j] = 'extra';
} else {
d[i][j] = left;
bt[i][j] = 'missing';
}
let diag = val(i - 1, j - 1);
if (actual[i] === expected[j] && d[i][j] < diag + 1) {
d[i][j] = diag + 1;
bt[i][j] = 'eq';
}
}
}
// Backtrack results.
let i = N - 1;
let j = M - 1;
let missing = [];
let extra = [];
let equal = [];
while (i >= 0 && j >= 0) {
switch (bt[i][j]) {
case 'extra':
extra.push(actual[i]);
i -= 1;
break;
case 'missing':
missing.push(expected[j]);
j -= 1;
break;
case 'eq':
equal.push(actual[i]);
i -= 1;
j -= 1;
break;
}
}
while (i >= 0)
extra.push(actual[i--]);
while (j >= 0)
missing.push(expected[j--]);
extra.reverse();
missing.reverse();
equal.reverse();
return {extra, missing, equal};
function val(i, j) {
return i < 0 || j < 0 ? 0 : d[i][j];
}
}
module.exports = Documentation;

View File

@ -1,3 +1,5 @@
const fs = require('fs');
const path = require('path');
const esprima = require('esprima');
const ESTreeWalker = require('../../third_party/chromium/ESTreeWalker');
const Documentation = require('./Documentation');
@ -22,13 +24,17 @@ class JSOutline {
_onClassDeclaration(node) {
this._flushClassIfNeeded();
this._currentClassName = this._getIdentifier(node.id);
this._currentClassName = this._extractText(node.id);
}
_onMethodDefinition(node) {
console.assert(this._currentClassName !== null);
let methodName = this._getIdentifier(node.key);
let method = new Documentation.Method(methodName);
console.assert(node.value.type === 'FunctionExpression');
const args = [];
for (let param of node.value.params)
args.push(new Documentation.Argument(this._extractText(param)));
let methodName = this._extractText(node.key);
let method = new Documentation.Method(methodName, args);
this._currentClassMethods.push(method);
}
@ -41,12 +47,27 @@ class JSOutline {
this._currentClassMethods = [];
}
_getIdentifier(node) {
_extractText(node) {
if (!node)
return null;
let text = this._text.substring(node.range[0], node.range[1]).trim();
return /^[$A-Z_][0-9A-Z_$]*$/i.test(text) ? text : null;
return text;
}
}
module.exports = JSOutline;
/**
* @param {!Array<string>} dirPath
* @return {!Promise<!Documentation>}
*/
module.exports = async function(dirPath) {
let filePaths = fs.readdirSync(dirPath)
.filter(fileName => fileName.endsWith('.js'))
.map(fileName => path.join(dirPath, fileName));
let classes = [];
for (let filePath of filePaths) {
let outline = new JSOutline(fs.readFileSync(filePath, 'utf8'));
classes.push(...outline.classes);
}
return new Documentation(classes);
};

126
test/doclint/MDBuilder.js Normal file
View File

@ -0,0 +1,126 @@
const fs = require('fs');
const markdownToc = require('markdown-toc');
const path = require('path');
const Documentation = require('./Documentation');
const commonmark = require('commonmark');
const Browser = require('../../lib/Browser');
class MDOutline {
/**
* @param {!Browser} browser
* @param {string} text
* @return {!MDOutline}
*/
static async create(browser, text) {
// Render markdown as HTML.
const reader = new commonmark.Parser();
const parsed = reader.parse(text);
const writer = new commonmark.HtmlRenderer();
const html = writer.render(parsed);
// Extract headings.
const page = await browser.newPage();
await page.setContent(html);
const classes = await page.evaluate(() => {
let classes = [];
let currentClass = {};
let method = {};
for (let element of document.body.querySelectorAll('h3, h4, h4 + ul > li')) {
if (element.matches('h3')) {
currentClass = {
name: element.textContent,
methods: [],
};
classes.push(currentClass);
} else if (element.matches('h4')) {
method = {
name: element.textContent,
args: []
};
currentClass.methods.push(method);
} else if (element.matches('li') && element.firstChild.matches && element.firstChild.matches('code')) {
method.args.push(element.firstChild.textContent);
}
}
return classes;
});
return new MDOutline(classes);
}
constructor(classes) {
this.classes = [];
this.errors = [];
const classHeading = /^class: (\w+)$/;
const constructorRegex = /^new (\w+)\((.*)\)$/;
const methodRegex = /^(\w+)\.(\w+)\((.*)\)$/;
let currentClassName = null;
let currentClassMethods = [];
for (const cls of classes) {
let match = cls.name.match(classHeading);
currentClassName = match[1];
for (let mdMethod of cls.methods) {
let className = null;
let methodName = null;
let parameters = null;
let title = mdMethod.name;
if (constructorRegex.test(title)) {
let match = title.match(constructorRegex);
className = match[1];
parameters = match[2];
methodName = 'constructor';
} else if (methodRegex.test(title)) {
let match = title.match(methodRegex);
className = match[1];
methodName = match[2];
parameters = match[3];
}
if (!currentClassName || !className || !methodName || className.toLowerCase() !== currentClassName.toLowerCase()) {
console.warn('failed to process header as method: ' + mdMethod.name);
continue;
}
parameters = parameters.trim().replace(/[\[\]]/g, '');
if (parameters !== mdMethod.args.join(', '))
this.errors.push(`Heading arguments for "${mdMethod.name}" do not match described ones, i.e. "${parameters}" != "${mdMethod.args.join(', ')}"`);
let args = mdMethod.args.map(arg => new Documentation.Argument(arg));
let method = new Documentation.Method(methodName, args);
currentClassMethods.push(method);
}
flushClassIfNeeded.call(this);
}
function flushClassIfNeeded() {
if (currentClassName === null)
return;
this.classes.push(new Documentation.Class(currentClassName, currentClassMethods));
currentClassName = null;
currentClassMethods = [];
}
}
}
/**
* @param {!Array<string>} dirPath
* @return {!Promise<{documentation: !Documentation, errors: !Array<string>}>}
*/
module.exports = async function(dirPath) {
let filePaths = fs.readdirSync(dirPath)
.filter(fileName => fileName.endsWith('.md'))
.map(fileName => path.join(dirPath, fileName));
let classes = [];
let errors = [];
const browser = new Browser({args: ['--no-sandbox']});
for (let filePath of filePaths) {
const markdownText = fs.readFileSync(filePath, 'utf8');
const newMarkdownText = markdownToc.insert(markdownText);
if (markdownText !== newMarkdownText)
errors.push('Markdown TOC is outdated, run `yarn generate-toc`');
let outline = await MDOutline.create(browser, markdownText);
classes.push(...outline.classes);
errors.push(...outline.errors);
}
await browser.close();
const documentation = new Documentation(classes);
return { documentation, errors };
};

View File

@ -1,81 +0,0 @@
const Documentation = require('./Documentation');
const commonmark = require('commonmark');
const Browser = require('../../lib/Browser');
class MDOutline {
/**
* @return {!MDOutline}
*/
static async create(text) {
// Render markdown as HTML.
const reader = new commonmark.Parser();
const parsed = reader.parse(text);
const writer = new commonmark.HtmlRenderer();
const html = writer.render(parsed);
// Extract headings.
const browser = new Browser({args: ['--no-sandbox']});
const page = await browser.newPage();
await page.setContent(html);
const headings = await page.evaluate(() => {
let headings = {};
let methods = [];
for (let header of document.body.querySelectorAll('h3,h4')) {
if (header.matches('h3')) {
methods = [];
headings[header.textContent] = methods;
} else {
methods.push(header.textContent);
}
}
return headings;
});
await browser.close();
return new MDOutline(headings);
}
constructor(headings) {
this.classes = [];
const classHeading = /^class: (\w+)$/;
const constructorRegex = /^new (\w+)\((.*)\)$/;
const methodRegex = /^(\w+)\.(\w+)\((.*)\)$/;
let currentClassName = null;
let currentClassMethods = [];
for (const heading of Object.keys(headings)) {
let match = heading.match(classHeading);
currentClassName = match[1];
for (const title of headings[heading]) {
let className = null;
let methodName = null;
if (constructorRegex.test(title)) {
let match = title.match(constructorRegex);
className = match[1];
methodName = 'constructor';
} else if (methodRegex.test(title)) {
let match = title.match(methodRegex);
className = match[1];
methodName = match[2];
}
if (!currentClassName || !className || !methodName || className.toLowerCase() !== currentClassName.toLowerCase()) {
console.warn('failed to process header as method: ' + heading);
continue;
}
let method = new Documentation.Method(methodName);
currentClassMethods.push(method);
}
flushClassIfNeeded.call(this);
}
function flushClassIfNeeded() {
if (currentClassName === null)
return;
this.classes.push(new Documentation.Class(currentClassName, currentClassMethods));
currentClassName = null;
currentClassMethods = [];
}
}
}
module.exports = MDOutline;

View File

@ -1,12 +1,9 @@
const fs = require('fs');
const path = require('path');
const JSOutline = require('./JSOutline');
const MDOutline = require('./MDOutline');
const jsBuilder = require('./JSBuilder');
const mdBuilder = require('./MDBuilder');
const Documentation = require('./Documentation');
const markdownToc = require('markdown-toc');
const PROJECT_DIR = path.join(__dirname, '..', '..');
const apiMdText = fs.readFileSync(path.join(PROJECT_DIR, 'docs', 'api.md'), 'utf8');
let EXCLUDE_CLASSES = new Set([
'Connection',
@ -30,64 +27,65 @@ let EXCLUDE_METHODS = new Set([
'Response.constructor',
]);
// Build up documentation from JS sources.
let jsClassesArray = [];
let files = fs.readdirSync(path.join(PROJECT_DIR, 'lib'));
for (let file of files) {
if (!file.endsWith('.js'))
continue;
let filePath = path.join(PROJECT_DIR, 'lib', file);
let outline = new JSOutline(fs.readFileSync(filePath, 'utf8'));
// Filter out private classes and methods.
for (let cls of outline.classes) {
/**
* @param {!Documentation} jsDocumentation
* @return {!Documentation}
*/
function filterJSDocumentation(jsDocumentation) {
// Filter classes and methods.
let classes = [];
for (let cls of jsDocumentation.classesArray) {
if (EXCLUDE_CLASSES.has(cls.name))
continue;
let methodsArray = cls.methodsArray.filter(method => {
let methods = cls.methodsArray.filter(method => {
if (method.name.startsWith('_'))
return false;
let shorthand = `${cls.name}.${method.name}`;
return !EXCLUDE_METHODS.has(shorthand);
return !EXCLUDE_METHODS.has(`${cls.name}.${method.name}`);
});
jsClassesArray.push(new Documentation.Class(cls.name, methodsArray));
classes.push(new Documentation.Class(cls.name, methods));
}
return new Documentation(classes);
}
let mdClassesArray;
let jsDocumentation;
let mdDocumentation;
let mdParseErrors;
let diff;
beforeAll(SX(async function() {
// Build up documentation from MD sources.
let mdOutline = await MDOutline.create(apiMdText);
mdClassesArray = mdOutline.classes;
jsDocumentation = filterJSDocumentation(await jsBuilder(path.join(PROJECT_DIR, 'lib')));
let mdDoc = await mdBuilder(path.join(PROJECT_DIR, 'docs'));
mdDocumentation = mdDoc.documentation;
mdParseErrors = mdDoc.errors;
diff = Documentation.diff(mdDocumentation, jsDocumentation);
}));
describe('table of contents', function() {
it('should match markdown-toc\'s output', () => {
const newApiMdText = markdownToc.insert(apiMdText);
if (apiMdText !== newApiMdText)
fail('markdown TOC is outdated, run `yarn generate-toc`');
});
});
// Compare to codebase.
describe('api.md', function() {
let mdClasses = new Map();
let jsClasses = new Map();
it('MarkDown should not contain any duplicate classes', () => {
for (let mdClass of mdClassesArray) {
if (mdClasses.has(mdClass.name))
fail(`Documentation has duplicate declaration of ${mdClass.name}`);
mdClasses.set(mdClass.name, mdClass);
}
});
it('JavaScript should not contain any duplicate classes (probably error in parsing!)', () => {
for (let jsClass of jsClassesArray) {
describe('JavaScript documentation parser', function() {
it('should not contain any duplicate classes (probably error in parsing!)', () => {
let jsClasses = new Map();
for (let jsClass of jsDocumentation.classesArray) {
if (jsClasses.has(jsClass.name))
fail(`JavaScript has duplicate declaration of ${jsClass.name}. (This probably means that this linter has an error)`);
jsClasses.set(jsClass.name, jsClass);
}
});
});
describe('Markdown Documentation', function() {
it('should not have any parse errors', () => {
for (let error of mdParseErrors)
fail(error);
});
it('should not contain any duplicate classes', () => {
let mdClasses = new Map();
for (let mdClass of mdDocumentation.classesArray) {
if (mdClasses.has(mdClass.name))
fail(`Documentation has duplicate declaration of class ${mdClass.name}`);
mdClasses.set(mdClass.name, mdClass);
}
});
it('class constructors should be defined before other methods', () => {
for (let mdClass of mdClasses.values()) {
for (let mdClass of mdDocumentation.classesArray) {
let constructorMethod = mdClass.methods.get('constructor');
if (!constructorMethod)
continue;
@ -96,7 +94,7 @@ describe('api.md', function() {
}
});
it('methods should be sorted alphabetically', () => {
for (let mdClass of mdClasses.values()) {
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')
@ -109,37 +107,29 @@ describe('api.md', function() {
}
});
it('should not contain any non-existing class', () => {
for (let mdClass of mdClasses.values()) {
if (!jsClasses.has(mdClass.name))
fail(`Documentation describes non-existing class ${mdClass.name}`);
}
for (let className of diff.extraClasses)
fail(`Documentation describes non-existing class ${className}`);
});
it('should describe all existing classes', () => {
for (let jsClass of jsClasses.values()) {
if (!mdClasses.has(jsClass.name))
fail(`Documentation lacks description of class ${jsClass.name}`);
}
for (let className of diff.missingClasses)
fail(`Documentation lacks description of class ${className}`);
});
it('should not contain any non-existing methods', () => {
for (let mdClass of mdClasses.values()) {
let jsClass = jsClasses.get(mdClass.name);
if (!jsClass)
continue;
for (let method of mdClass.methods.values()) {
if (!jsClass.methods.has(method.name))
fail(`Documentation describes non-existing method: ${jsClass.name}.${method.name}()`);
}
}
for (let methodName of diff.extraMethods)
fail(`Documentation describes non-existing method: ${methodName}`);
});
it('should describe all existing methods', () => {
for (let jsClass of jsClasses.values()) {
let mdClass = mdClasses.get(jsClass.name);
if (!mdClass)
continue;
for (let method of jsClass.methods.values()) {
if (!mdClass.methods.has(method.name))
fail(`Documentation lacks ${jsClass.name}.${method.name}()`);
}
for (let methodName of diff.missingMethods)
fail(`Documentation lacks method ${methodName}`);
});
it('should describe all arguments propertly', () => {
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}"`);
fail(text.join('\n'));
}
});
});