Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

comment all errors, prevent duplicate rules #10

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

comment all errors, prevent duplicate rules #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b1aecbf
comment all errors, prevent duplicate rules
baruchadi Apr 28, 2022
3a20daf
apply eslint styles
baruchadi May 19, 2022
187ea11
remove package-lock, update yarn.lock
baruchadi May 19, 2022
1a91b0f
fix yarn.lock
baruchadi May 19, 2022
28241e3
make files required again
baruchadi May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions bin/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,50 @@ const eslintBankruptcy = require('..');
require('hard-rejection/register');

require('yargs')
.command('$0 <files...>', '', yargs => {
yargs.positional('files', {
describe: 'Files to modify',
string: true,
required: true
baruchadi marked this conversation as resolved.
Show resolved Hide resolved
});
}, main)
.command(
'$0 <files...>',
'',
yargs => {
yargs.positional('files', {
describe: 'Files to modify',
string: true,
required: true
});
},
main
)
.options({
rule: {
alias: 'r',
array: true,
string: true,
demandOption: true,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed demandOption from rule, if rule is not provided - run this for any rule

description: 'The rule to disable. Pass this flag multiple times to disable multiple rules at once.'
description:
'The rule to disable. Pass this flag multiple times to disable multiple rules at once.'
},
dry: {
alias: 'd',
boolean: true,
description: 'If true, print a description of which files will be updated, but do not actually change anything.'
description:
'If true, print a description of which files will be updated, but do not actually change anything.'
},
explanation: {
alias: 'e',
string: true,
description: 'Highly recommended. A message that will be included with the disable comments.'
description:
'Highly recommended. A message that will be included with the disable comments.'
},
eslintOutputFilePath: {
string: true,
description: 'Pass the output of `eslint --format json`. ' +
description:
'Pass the output of `eslint --format json`. ' +
'Use this if your project has a special eslint setup, or you want to preprocess what this tool runs on.'
}
})
.strict()
.argv;
.strict().argv;

/**
* I'm not sure how to these types flow automatically.
* @param {Record<'files' | 'rule' | 'dry' | 'explanation' | 'eslintOutputFilePath', any>} argv
* @param {Record<'files' | 'rule' | 'dry' | 'explanation' | 'eslintOutputFilePath', any>} argv
*/
async function main(argv) {
if (!argv.files.length) {
Expand Down
133 changes: 80 additions & 53 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,30 +11,36 @@ const writeFile = util.promisify(fs.writeFile.bind(fs));
const log = require('./src/log');

/**
*
* @param {string[]} files
* @param {string | undefined} eslintOutputFilePath
* @returns
*
* @param {string[]} files
* @param {string | undefined} eslintOutputFilePath
* @returns
*/
async function getEslintReport(files, eslintOutputFilePath) {
if (eslintOutputFilePath) {
log.debug({eslintOutputFilePath}, 'Reading eslint output from JSON instead of spawning eslint.');
log.debug(
{eslintOutputFilePath},
'Reading eslint output from JSON instead of spawning eslint.'
);
return loadJsonFile(eslintOutputFilePath);
}
const eslintBin = await getEslintBinPath();
return runEslint(eslintBin, files);
}

/**
* @param {object} options
* @param {object} options
* @param {string[]} options.files
* @param {string[]} options.rules
* @param {boolean | undefined} options.dry
* @param {string | undefined} options.explanation
* @param {string} options.eslintOutputFilePath
*/
async function eslintBankruptcy(options) {
const eslintReport = await getEslintReport(options.files, options.eslintOutputFilePath);
const eslintReport = await getEslintReport(
options.files,
options.eslintOutputFilePath
);
const violations = await getViolations(eslintReport, options.rules);
const countViolatingFiles = _.size(violations);
const logParams = {countViolatingFiles};
Expand All @@ -45,7 +51,7 @@ async function eslintBankruptcy(options) {
log.trace({violations});
}
log.info(logParams, `Found violations in ${countViolatingFiles} files.`);

if (options.dry) {
log.info('Exiting because dry mode is on.');
return;
Expand All @@ -55,72 +61,86 @@ async function eslintBankruptcy(options) {
}

/**
* @param {ReturnType<typeof getViolations>} changes
* @param {ReturnType<typeof getViolations>} changes
* @param {string | undefined} explanation
*/
function insertComments(changes, explanation) {
// @codemod/cli has more functionality, but it'll be painful to use because we'd have to run it in subproc.
// Our set of changes to make is in memory, so passing that through to the transform would also be a pain.

return Promise.all(_.map(changes, (violations, filePath) => insertCommentsInFile(filePath, violations, explanation)));
return Promise.all(
_.map(changes, (violations, filePath) =>
insertCommentsInFile(filePath, violations, explanation)
)
);
}

/**
* @param {string} filePath
* @param {{[line: number]: string[]}} violations
* @param {string | undefined} explanation
* @param {string} filePath
* @param {{[line: number]: string[]}} violations
* @param {string | undefined} explanation
*/
async function insertCommentsInFile(filePath, violations, explanation) {
log.info({filePath}, 'Modifying file');
// I wonder if the line splitting is too naive here.
const inputCode = (await readFile(filePath, 'utf8')).split('\n');

// I would declare this inline if I knew how to use the TS JSDoc syntax with it.
/** @type {string[]} */
const initial = [];

const outputCode = inputCode.reduce((acc, line, lineIndex) => {
const toAppend = [];
// +1 because ESLint gives the line numbers 1-indexed.
const violation = violations[lineIndex + 1];
if (violation) {
const leadingWhitespaceLength = line.length - line.trimLeft().length;
const leadingWhitespace = line.substring(0, leadingWhitespaceLength);
if (explanation) {
toAppend.push(`${leadingWhitespace}// ${explanation}`);

const outputCode = inputCode
.reduce((acc, line, lineIndex) => {
const toAppend = [];
// +1 because ESLint gives the line numbers 1-indexed.
const violation = violations[lineIndex + 1];
if (violation) {
const leadingWhitespaceLength = line.length - line.trimLeft().length;
const leadingWhitespace = line.substring(0, leadingWhitespaceLength);
if (explanation) {
toAppend.push(`${leadingWhitespace}/* ${explanation} */`);
}
toAppend.push(leadingWhitespace + getEslintDisableComment(violation));
}
toAppend.push(leadingWhitespace + getEslintDisableComent(violation));
}
toAppend.push(line);
return [...acc, ...toAppend];
}, initial).join('\n');
toAppend.push(line);
return [...acc, ...toAppend];
}, initial)
.join('\n');

log.trace({outputCode, filePath});
await writeFile(filePath, outputCode);
}

/**
* @param {string[]} rules
* @param {string[]} rules
*/
function getEslintDisableComent(rules) {
return `// eslint-disable-next-line ${rules.join(' ')}`;
function getEslintDisableComment(rules) {
const ruleSet = new Set(rules);
return `/* eslint-disable-next-line ${Array.from(ruleSet).join(', ')} */`;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using /* ... */ style comments made it easier to use regex to look for the new comments and fix broken comments in jsx

}

/**
* @param {Array<{filePath: string, messages: Array<{ruleId: string, line: number}>}>} eslintReport
* @param {Array<{filePath: string, messages: Array<{ruleId: string, line: number}>}>} eslintReport
* @param {string[]} rules
* @return {{[filePath: string]: {[lineNumber: number]: string[]}}}}
* @returns{{[filePath: string]: {[lineNumber: number]: string[]}}}}
*/
function getViolations(eslintReport, rules) {
return _(eslintReport)
.flatMapDeep(({filePath, messages}) => _.flatMap(messages, ({ruleId, line}) => ({filePath, ruleId, line})))
.groupBy('filePath')
.mapValues(entry => _(entry)
.filter(({ruleId}) => rules.includes(ruleId))
.groupBy('line')
.mapValues(violations => _.map(violations, 'ruleId'))
.value()
.flatMapDeep(({filePath, messages}) =>
_.flatMap(messages, ({ruleId, line}) => ({filePath, ruleId, line}))
)
.groupBy('filePath')
.mapValues(entry => {
let _entry = _(entry);
if (rules != null && rules.length > 0) {
_entry = _entry.filter(({ruleId}) => rules.includes(ruleId));
}

return _entry
.groupBy('line')
.mapValues(violations => _.map(violations, 'ruleId'))
.value();
})
.toPairs()
.filter(([, violations]) => Boolean(_.size(violations)))
.fromPairs()
Expand All @@ -131,38 +151,45 @@ async function getEslintBinPath(dirPath = process.cwd()) {
const eslintMainPath = resolveFrom(dirPath, 'eslint');
const eslintRoot = await findParentDir(eslintMainPath, 'package.json');
if (!eslintRoot) {
throw new Error('eslint-bankruptcy could not find an eslint instance to run. ' +
// The rule is over-zealous.
// eslint-disable-next-line quotes
`To resolve this, run this command from a directory in which "require('eslint')" works.`);
throw new Error(
'eslint-bankruptcy could not find an eslint instance to run. ' +
// The rule is over-zealous.
// eslint-disable-next-line quotes
`To resolve this, run this command from a directory in which "require('eslint')" works.`
);
}
const packageJsonPath = path.join(eslintRoot, 'package.json');
/** @type {{bin: {eslint: string}}} */
/** @type {{bin: {eslint: string}}} */
const packageJson = await loadJsonFile(packageJsonPath);
return path.resolve(eslintRoot, packageJson.bin.eslint);
}

/**
*
* @param {string} eslintBinPath
* @param {string[]} files
*
* @param {string} eslintBinPath
* @param {string[]} files
*/
async function runEslint(eslintBinPath, files) {
log.debug({eslintBinPath, files}, 'Spawning eslint');

/**
* @param {Error} spawnError
* @param {Error} spawnError
*/
function throwESLintFailedError(spawnError) {
const err = new Error(
'Eslint did not run successfully. Did you run this command from within the project ' +
"you're trying to transform? This is necessary so Eslint can load your project's config.");
"you're trying to transform? This is necessary so Eslint can load your project's config."
);
Object.assign(err, {eslintBinPath, files, spawnError});
throw err;
}

try {
const {stdout, stderr} = await execa(eslintBinPath, [...files, '--format', 'json']);
const {stdout, stderr} = await execa(eslintBinPath, [
...files,
'--format',
'json'
]);
log.debug({stdout, stderr});
} catch (e) {
if (!e.code || e.code === 1) {
Expand All @@ -177,4 +204,4 @@ async function runEslint(eslintBinPath, files) {
}
}

module.exports = eslintBankruptcy;
module.exports = eslintBankruptcy;