Linting for translators

It looks a little baroque, I know, but the current implementation achieves a few goals I think are interesting: * First of all, it allows --fix * It does this while retaining the processor. Retaining the processor means that vscode and other editors that have eslint support can load translators -- otherwise they would error out on the header. * No temporary files * Monkey-patches notice/notice so it can --fix a missing license comment using information from the header Fix inside editors is not (currently) possible because they must load the plugin processor; eslint [does not support](eslint/eslint#5121) the fix/processor combo, and looking at that discussion, it does not seem it will.
adam3smith · Mar 28, 2019 · 9427b33fe8ec76b32a0a9551df290b5acc7b71d3 · 9427b33
1 parent 91a6f9d
commit 9427b33fe8ec76b32a0a9551df290b5acc7b71d3
diff --git a/.ci/.eslintrc b/.ci/.eslintrc
@@ -0,0 +1,11 @@
+  
+{
+  
+	"env": {
+  
+		"node": true,
+  
+	},
+  
+	"parserOptions": {
+  
+		"ecmaVersion": 2018
+  
+	},
+  
+	"rules": {
+  
+		"notice/notice": "off"
+  
+	}
+  
+}
diff --git a/.ci/AGPL b/.ci/AGPL
@@ -0,0 +1,22 @@
+  
+/*
+  
+	***** BEGIN LICENSE BLOCK *****
+  
+
+  
+	Copyright © ${ period } ${ holder }
+  
+
+  
+	This file is part of Zotero.
+  
+
+  
+	Zotero is free software: you can redistribute it and/or modify
+  
+	it under the terms of the GNU Affero General Public License as published by
+  
+	the Free Software Foundation, either version 3 of the License, or
+  
+	(at your option) any later version.
+  
+
+  
+	Zotero is distributed in the hope that it will be useful,
+  
+	but WITHOUT ANY WARRANTY; without even the implied warranty of
+  
+	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+  
+	GNU Affero General Public License for more details.
+  
+
+  
+	You should have received a copy of the GNU Affero General Public License
+  
+	along with Zotero. If not, see <http://www.gnu.org/licenses/>.
+  
+
+  
+	***** END LICENSE BLOCK *****
+  
+*/
diff --git a/.ci/eslint-plugin-zotero-translator/bin/teslint.js b/.ci/eslint-plugin-zotero-translator/bin/teslint.js
@@ -0,0 +1,111 @@
+  
+#!/usr/bin/env node
+  
+
+  
+'use strict';
+  
+
+  
+const fs = require('fs');
+  
+const path = require('path');
+  
+const find = require('recursive-readdir-synchronous');
+  
+const CLIEngine = require("eslint").CLIEngine;
+  
+const argv = require('commander');
+  
+
+  
+const translators = require('../lib/translators');
+  
+
+  
+argv
+  
+	.version(CLIEngine.version)
+  
+	.option('-f, --fix', 'Automatically fix problems')
+  
+	.option('--no-ignore', 'Disable use of ignore files and patterns')
+  
+	.option('--quiet', 'Report errors only - default: false')
+  
+	.option('--dump-decorated [file]', 'Dump decorated translator to file for inspection')
+  
+	.parse(process.argv);
+  
+
+  
+/* PATCHES */
+  
+// disable the processor so that fixing works
+  
+const eslintPluginZoteroTranslator = require('eslint-plugin-zotero-translator');
+  
+delete eslintPluginZoteroTranslator.processors;
+  
+
+  
+/* MAIN */
+  
+// split sources to lint into regular javascript (handled by executeOnFiles) and translators (handled by executeOnText)
+  
+const sources = {
+  
+	javascripts: [],
+  
+	translators: [],
+  
+	errors: 0,
+  
+};
+  
+function findIgnore(file, stats) {
+  
+	if (stats.isDirectory()) return (path.basename(file) == "node_modules");
+  
+	return !file.endsWith('.js');
+  
+}
+  
+for (const target of argv.args) {
+  
+	if (!fs.existsSync(target)) continue;
+  
+	const files = fs.lstatSync(target).isDirectory() ? find(target, [findIgnore]) : [target];
+  
+	for (const file of files) {
+  
+		if (path.dirname(path.resolve(file)) === translators.cache.repo) {
+  
+			const translator = translators.cache.get(file);
+  
+			if (translator.header) {
+  
+				translator.filename = file;
+  
+				sources.translators.push(translator);
+  
+			}
+  
+			else {
+  
+				sources.javascripts.push(file);
+  
+			}
+  
+		}
+  
+		else {
+  
+			sources.javascripts.push(file);
+  
+		}
+  
+	}
+  
+}
+  
+
+  
+const cli = new CLIEngine({
+  
+	cwd: translators.cache.repo,
+  
+	fix: argv.fix,
+  
+	ignore: argv.ignore, // otherwise you can't lint stuff in hidden dirs
+  
+});
+  
+const formatter = cli.getFormatter();
+  
+function showResults(files, results) {
+  
+	if (argv.quiet) results = CLIEngine.getErrorResults(results);
+  
+	for (const res of results) {
+  
+		sources.errors += res.errorCount;
+  
+	}
+  
+
+  
+	if (results.length) {
+  
+		console.log(formatter(results)); // eslint-disable-line no-console
+  
+	}
+  
+	else {
+  
+		if (Array.isArray(files)) files = files.join(', ');
+  
+		if (!argv.quiet) console.log(files, 'OK'); // eslint-disable-line no-console
+  
+	}
+  
+}
+  
+
+  
+if (sources.javascripts.length) {
+  
+	const report = cli.executeOnFiles(sources.javascripts);
+  
+	if (argv.fix) {
+  
+		for (const result of report.results) {
+  
+			if (result.messages.find(msg => msg.ruleId === 'notice/notice' && msg.fix)) {
+  
+				console.log(`Not safe to apply 'notice/notice' to ${result.filePath}`); // eslint-disable-line no-console
+  
+				process.exit(1); // eslint-disable-line no-process-exit
+  
+			}
+  
+		}
+  
+		CLIEngine.outputFixes(report);
+  
+	}
+  
+	showResults(sources.javascripts, report.results);
+  
+}
+  
+
+  
+for (const translator of sources.translators) {
+  
+	if (argv.dumpDecorated) fs.writeFileSync(argv.dumpDecorated, translator.source, 'utf-8');
+  
+	const report = cli.executeOnText(translator.source, translator.filename);
+  
+	if (argv.fix) {
+  
+		for (const result of report.results) {
+  
+			if (result.output) {
+  
+				try {
+  
+					fs.writeFileSync(result.filePath, translators.strip(result.output), 'utf-8');
+  
+				}
+  
+				catch (err) {
+  
+					console.log(`Error writing fixed ${result.filePath}: ${err.message}`); // eslint-disable-line no-console
+  
+					process.exit(1); // eslint-disable-line no-process-exit
+  
+				}
+  
+			}
+  
+		}
+  
+	}
+  
+	showResults(translator.filename, report.results);
+  
+}
+  
+
+  
+process.exit(sources.errors); // eslint-disable-line no-process-exit
diff --git a/.ci/eslint-plugin-zotero-translator/index.js b/.ci/eslint-plugin-zotero-translator/index.js
@@ -2,188 +2,14 @@
 * @fileoverview Checks Zotero translators for errors and recommended style
 * @author Emiliano Heyns
 */
-  
-"use strict";
-  
-const fs = require('fs');
-  
-const path = require('path');
+  
+'use strict';
-  
-const header_var_name = '__eslint_zotero_translator_header';
+  
+const requireDir = require('require-dir');
-  
-const headers = {};
-  
-const deleted = new Set(
-  
-	fs.readFileSync('deleted.txt', 'utf-8')
-  
-		.split('\n')
-  
-		.map(line => line.split(' ')[0])
-  
-		.filter(id => id && id.indexOf('-') > 0)
-  
-);
-  
-
-  
-module.exports.processors = {
-  
-	'.js': {
-  
-		preprocess: function(text, filename) {
-  
-			filename = path.resolve(filename);
-  
-
-  
-			headers[filename] = {
-  
-				// capture header
-  
-				raw: text.replace(/\n}\n[\S\s]*/, '\n}'),
-  
-			};
-  
-
-  
-			try {
-  
-				headers[filename].parsed = JSON.parse(headers[filename].raw);
-  
-
-  
-			} catch (err) {
-  
-				headers[filename].error = err.message; // leave for rules to pick up
-  
-			}
-  
-
-  
-			text = `var ${header_var_name} = ` // assign header to variable to make valid JS
-  
-				+ text
-  
-					.replace('\n', ' // eslint-disable-line no-unused-vars \n') // prevent eslint warnings about this variable being unused
-  
-					.replace('\n}\n', '\n};\n'); // add a semicolon after the header to pacify eslint
-  
-
-  
-			// fs.writeFileSync(header_var_name + '.js', text, 'utf-8')
-  
-
-  
-			return [text];
-  
-		},
-  
-
-  
-		// takes a Message[][] and filename
-  
-		postprocess: function(messages, filename) {
-  
-			return messages[0].sort((a, b) => {
-  
-				const la = a.line || 0;
-  
-				const lb = b.line || 0;
-  
-
-  
-				return (la !== lb) ? la - lb : a.ruleId.localeCompare(b.ruleId);
-  
-			});
-  
-		},
-  
-	},
-  
-};
-  
-
-  
-
-  
-module.exports.rules = {
-  
-	'header-valid-json': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-				const header = headers[filename];
-  
-
-  
-				if (!header) {
-  
-					context.report(node, 'Header not parsed');
-  
-
-  
-				} else if (header.error) {
-  
-					context.report(node, `Could not parse header: ${header.error}`);
-  
-
-  
-				}
-  
-			}
-  
-		};
-  
-	},
-  
-
-  
-	'last-updated': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-				const header = headers[filename];
-  
-
-  
-				if (!header || header.error) return; // picked up by valid-json
-  
-
-  
-				if (!header.parsed.lastUpdated) {
-  
-					context.report(node, 'Header needs lastUpdated field');
-  
-
-  
-				} /* else {
-  
-					// disabled until I figure out something smart -- git doesn't retain file modification dates, and it's too involved to hit the github API all the time
-  
-					const stats = fs.statSync(filename)
-  
-					if (stats.mtime > (new Date(header.parsed.lastUpdated))) {
-  
-						context.report(node, 'lastUpdated field is older than file modification time');
-  
-					}
-  
-				}
-  
-				*/
-  
-			}
-  
-		};
-  
-	},
-  
-
-  
-	// this is a very simplistic rule to find 'for each' until I find a better eslint plugin that does this
-  
-	'no-for-each': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-
-  
-				let lineno = 0;
-  
-				let m
-  
-				for (const line of fs.readFileSync(filename, 'utf-8').split('\n')) {
-  
-					lineno += 1;
-  
-
-  
-					if (m = line.match(/for each *\(/)) {
-  
-						context.report({
-  
-							node,
-  
-							message: "Deprecated JavaScript 'for each' statement",
-  
-							loc: { start: { line: lineno, column: line.indexOf(m[0]) + 1 } },
-  
-						});
-  
-					}
-  
-				}
-  
-			}
-  
-		};
-  
-	},
-  
-
-  
-	'not-executable': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-
-  
-				try {
-  
-					fs.accessSync(filename, fs.constants.X_OK);
-  
-					context.report(node, `Translator '${path.basename(filename)}' should not be executable.`);
-  
-
-  
-				} catch(err) {
-  
-					return;
-  
-
-  
-				}
-  
-			}
-  
-		};
-  
-	},
-  
-
-  
-	// this is a very simplistic rule to find 'unnecesary use of indexOf' until I find a better eslint plugin that does this
-  
-	'prefer-index-of': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-
-  
-				let lineno = 0;
-  
-				let m
-  
-				for (const line of fs.readFileSync(filename, 'utf-8').split('\n')) {
-  
-					lineno += 1;
-  
-
-  
-					if (m = line.match(/\.indexOf(.*) *(=+ *-1|!=+ *-1|> *-1|>= *0|< *0)/)) {
-  
-						context.report({
-  
-							node,
-  
-							message: "Unnecessary '.indexOf()', use '.includes()' instead",
-  
-							loc: { start: { line: lineno, column: line.indexOf(m[0]) + 1 } },
-  
-						});
-  
-					}
-  
-				}
-  
-			}
-  
-		};
-  
-	},
-  
-
-  
-	'translator-id': function(context) {
-  
-		return {
-  
-			Program: function(node) {
-  
-				const filename = path.resolve(context.getFilename());
-  
-				const header = headers[filename];
-  
-
-  
-				if (!header || header.error) return; // picked up by valid-json
-  
-
-  
-				if (!header.parsed.translatorID) {
-  
-					context.report(node, 'Header has no translator ID');
-  
-
-  
-				} else if (deleted.has(header.parsed.translatorID)) {
-  
-					context.report(node, 'Header re-uses translator ID of deleted translator');
-  
-
-  
-				} else {
-  
-					for (const [other_filename, other_header] of Object.entries(headers)) {
-  
-						if (other_filename !== filename && other_header.translatorID === header.parsed.translatorID) {
-  
-							context.report(node, `Header re-uses translator ID of ${other_header.label}`);
-  
-							break;
-  
-						}
-  
-					}
-  
-				}
-  
-			}
-  
-		};
-  
-	},
+  
+module.exports = {
+  
+	rules: requireDir('./lib/rules'),
+  
+	processors: {
+  
+		'.js': require('./lib/processor'),
+  
+	}
 };
diff --git a/.ci/eslint-plugin-zotero-translator/lib/processor/index.js b/.ci/eslint-plugin-zotero-translator/lib/processor/index.js
@@ -0,0 +1,15 @@
+  
+'use strict';
+  
+
+  
+const translators = require('../translators').cache;
+  
+
+  
+module.exports = {
+  
+	preprocess: function (text, filename) {
+  
+		const translator = translators.get(filename);
+  
+
+  
+		return [(typeof translator.source === 'string') ? translator.source : text];
+  
+	},
+  
+
+  
+	postprocess: function (messages, _filename) {
+  
+		return messages[0];
+  
+	},
+  
+};
diff --git a/.ci/eslint-plugin-zotero-translator/lib/rules/header-valid-json.js b/.ci/eslint-plugin-zotero-translator/lib/rules/header-valid-json.js
@@ -0,0 +1,30 @@
+  
+'use strict';
+  
+
+  
+const translators = require('../translators').cache;
+  
+
+  
+module.exports = {
+  
+	meta: {
+  
+		type: 'problem',
+  
+		docs: {
+  
+			description: 'disallow invalid JSON in header',
+  
+			category: 'Possible Errors',
+  
+		},
+  
+	},
+  
+
+  
+	create: function (context) {
+  
+		return {
+  
+			Program: function (_node) {
+  
+				const translator = translators.get(context.getFilename());
+  
+
+  
+				if (!translator.source) return; // regular js source
+  
+
+  
+				if (translator.header.error) {
+  
+					context.report({
+  
+						message: `Could not parse header: ${translator.header.error.message}`,
+  
+						loc: { start: { line: translator.error.line, column: translator.error.column } },
+  
+					});
+  
+				}
+  
+			}
+  
+		};
+  
+	},
+  
+};