-
-
Notifications
You must be signed in to change notification settings - Fork 412
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.
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
Run linters with configurable concurrency #149
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c22f6f5
feat: Run linters with configurable concurrency
sudo-suhas 365d239
chore: Cleanup comment in test
sudo-suhas 8f9d1ff
test(pMap): Add tests for pMap in runScript
sudo-suhas 4c38c35
docs: Add docs for config options chunkSize, subTaskConcurrency
sudo-suhas File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,16 @@ | ||
{ | ||
"presets": ["es2015", "stage-0"] | ||
"presets": [ | ||
["env", { | ||
"targets": { | ||
"node": "current" | ||
} | ||
}] | ||
], | ||
"plugins": [ | ||
["transform-runtime", { | ||
"helpers": false, | ||
"polyfill": false, | ||
"regenerator": true | ||
}] | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
'use strict' | ||
|
||
/** | ||
* Calculates and returns the chunk size for given file paths and `chunkSize` | ||
* option. | ||
* | ||
* It returns the minimum of the following: | ||
* | ||
* - Total number of files | ||
* - Max allowed chunk size so that command length does not exceed the system | ||
* limitation on windows | ||
* - User specified chunk size or the default | ||
* | ||
* Worked example: | ||
* **Assumption** - Our max file path length is 100, Hence max allowed chunk | ||
* size is 80 | ||
* | ||
* - Case 1: Only 10 files are there, chunk size should be 10 only | ||
* - Case 2: There are 100 files and user has overridden the option with | ||
* chunk size 40. So chunk size should be 40 | ||
* - Case 3: There are 100 files and user has overridden the option with | ||
* chunk size 100. So chunk size should be 80 | ||
* | ||
* @param {Array<string>} paths The array of file paths | ||
* @param {number} idealChunkSize User specified / default chunk size | ||
* @returns {number} The chunk size | ||
*/ | ||
module.exports = function calcChunkSize(paths, idealChunkSize) { | ||
/* What is the longest file path? */ | ||
const maxPathLen = paths.reduce( | ||
(maxLen, filePath) => Math.max(maxLen, filePath.length), | ||
20 // safe initial value | ||
) | ||
|
||
/* In the worst case scenario, */ | ||
/* how many files can we process in a single command? */ | ||
/* For windows systems, command length is limited to 8192 */ | ||
const maxAllowedChunkSize = Math.floor(8000 / maxPathLen) | ||
|
||
/* Configured chunk size / default - idealChunkSize */ | ||
return Math.min(paths.length, maxAllowedChunkSize, idealChunkSize) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
'use strict' | ||
|
||
/** | ||
* Helper function for reading config option. | ||
* Returns the `config` option for given `key` or the given `defaultValue` | ||
* if `config` does not have the given `key`. | ||
* | ||
* @param {Object} config | ||
* @param {string} key | ||
* @param {*} defaultValue | ||
* @returns {*} | ||
*/ | ||
module.exports = function readConfigOption(config, key, defaultValue) { | ||
if (typeof config !== 'undefined' && typeof config[key] !== 'undefined') { | ||
return config[key] | ||
} | ||
|
||
return defaultValue | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,70 @@ | ||
'use strict' | ||
|
||
const findBin = require('./findBin') | ||
const chunk = require('lodash.chunk') | ||
const execa = require('execa') | ||
const pMap = require('p-map') | ||
|
||
const calcChunkSize = require('./calcChunkSize') | ||
const findBin = require('./findBin') | ||
const readConfigOption = require('./readConfigOption') | ||
|
||
module.exports = function runScript(commands, pathsToLint, packageJson, options) { | ||
const config = readConfigOption(options, 'config', {}) | ||
|
||
const concurrency = readConfigOption(config, 'subTaskConcurrency', 2) | ||
const chunkSize = calcChunkSize( | ||
pathsToLint, | ||
readConfigOption(config, 'chunkSize', Number.MAX_SAFE_INTEGER) | ||
) | ||
|
||
const filePathChunks = chunk(pathsToLint, chunkSize) | ||
|
||
const lintersArray = Array.isArray(commands) ? commands : [commands] | ||
|
||
return lintersArray.map(linter => ({ | ||
title: linter, | ||
task: () => { | ||
try { | ||
const res = findBin(linter, pathsToLint, packageJson, options) | ||
const res = findBin(linter, packageJson, options) | ||
|
||
// Only use gitDir as CWD if we are using the git binary | ||
// e.g `npm` should run tasks in the actual CWD | ||
const execaOptions = | ||
res.bin.endsWith('git') && options && options.gitDir ? { cwd: options.gitDir } : {} | ||
return new Promise((resolve, reject) => { | ||
execa(res.bin, res.args, execaOptions) | ||
.then(() => { | ||
resolve(`✅ ${ linter } passed!`) | ||
}) | ||
res.bin.endsWith('git') && options && options.gitDir | ||
? { cwd: options.gitDir } : {} | ||
|
||
const errors = [] | ||
const mapper = (pathsChunk) => { | ||
const args = res.args.concat(['--'], pathsChunk) | ||
|
||
return execa(res.bin, args, Object.assign({}, execaOptions)) | ||
/* If we don't catch, pMap will terminate on first rejection */ | ||
/* We want error information of all chunks */ | ||
.catch((err) => { | ||
reject(new Error(`🚫 ${ linter } found some errors. Please fix them and try committing again. | ||
${ err.stderr } | ||
${ err.stdout }`)) | ||
errors.push(err) | ||
}) | ||
}) | ||
} | ||
|
||
return pMap(filePathChunks, mapper, { concurrency }) | ||
.catch((err) => { | ||
/* This will probably never be called. But just in case.. */ | ||
throw new Error(`🚫 ${ linter } got an unexpected error. | ||
${ err.message }`) | ||
}) | ||
.then(() => { | ||
if (errors.length === 0) return `✅ ${ linter } passed!` | ||
|
||
const errStdout = errors.map(err => err.stdout).join('') | ||
const errStderr = errors.map(err => err.stderr).join('') | ||
|
||
throw new Error(`🚫 ${ linter } found some errors. Please fix them and try committing again. | ||
${ errStdout } | ||
${ errStderr }`) | ||
}) | ||
} catch (err) { | ||
throw err | ||
} | ||
} | ||
})) | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import calcChunkSize from '../src/calcChunkSize' | ||
|
||
// This is only ever used for length so the contents do not matter much | ||
const testFilePath = | ||
'This-is-only-ever-used-for-length-so-the-contents-do-not-matter-much.length-is-100-for-simplicity.js' | ||
|
||
describe('calcChunkSize', () => { | ||
it('should not return high chunk size for less files', () => { | ||
let chunkSize = calcChunkSize([testFilePath], 50) | ||
expect(chunkSize).toEqual(1) | ||
|
||
chunkSize = calcChunkSize([testFilePath, testFilePath], 50) | ||
expect(chunkSize).toEqual(2) | ||
}) | ||
|
||
it('should not return chunk size which will fail max command length', () => { | ||
const fakeFilePaths = Array(200).fill(testFilePath) | ||
const chunkSize = calcChunkSize(fakeFilePaths, Number.MAX_SAFE_INTEGER) | ||
expect(chunkSize).toEqual(80) | ||
}) | ||
|
||
it('should respect option chunkSize where ever possible', () => { | ||
const fakeFilePaths = Array(200).fill(testFilePath) | ||
const chunkSize = calcChunkSize(fakeFilePaths, 50) | ||
expect(chunkSize).toEqual(50) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import readConfigOption from '../src/readConfigOption' | ||
|
||
describe('readConfigOption', () => { | ||
|
||
it('should return default value if config is undefined', () => { | ||
const configOption = readConfigOption(undefined, 'my_key', 'default_value') | ||
expect(configOption).toEqual('default_value') | ||
}) | ||
|
||
it('should return default value if config option is undefined', () => { | ||
const configOption = readConfigOption({}, 'my_key', 'default_value') | ||
expect(configOption).toEqual('default_value') | ||
}) | ||
|
||
it('should return config option if not undefined', () => { | ||
const configOption = readConfigOption({ my_key: 'my_value' }, 'my_key', 'default_value') | ||
expect(configOption).toEqual('my_value') | ||
}) | ||
|
||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't covered with tests. Should we add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to add a mock right?