Skip to content

Commit

Permalink
fix: run all commands returned by function linters
Browse files Browse the repository at this point in the history
Function linters have to be evaluated at makeCmdTasks, and resolveTaskFn has to run for each of the returned commands.
  • Loading branch information
iiroj authored and okonet committed Jul 3, 2019
1 parent 146e6ce commit 0dd0c94
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 85 deletions.
38 changes: 16 additions & 22 deletions src/makeCmdTasks.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,6 @@ const resolveTaskFn = require('./resolveTaskFn')

const debug = require('debug')('lint-staged:make-cmd-tasks')

/**
* Get title for linter task. For a function, evaluate by passing single [file].
* For strings, return as-is
* @param {string|Function} linter
*/
const getTitle = linter => {
if (typeof linter === 'function') {
const resolved = linter(['[file]'])
return Array.isArray(resolved) ? resolved[0] : resolved
}
return linter
}

/**
* Creates and returns an array of listr tasks which map to the given commands.
*
Expand All @@ -27,16 +14,23 @@ const getTitle = linter => {
*/
module.exports = async function makeCmdTasks(commands, shell, gitDir, pathsToLint) {
debug('Creating listr tasks for commands %o', commands)
const commandsArray = Array.isArray(commands) ? commands : [commands]

return commandsArray.reduce((tasks, command) => {
// linter function may return array of commands that already include `pathsToLit`
const isFn = typeof command === 'function'
const resolved = isFn ? command(pathsToLint) : command
const linters = Array.isArray(resolved) ? resolved : [resolved] // Wrap non-array linter as array

const lintersArray = Array.isArray(commands) ? commands : [commands]
linters.forEach(linter => {
const task = {
title: linter,
task: resolveTaskFn({ gitDir, isFn, linter, pathsToLint, shell })
}

return lintersArray.map(linter => ({
title: getTitle(linter),
task: resolveTaskFn({
linter,
shell,
gitDir,
pathsToLint
tasks.push(task)
})
}))

return tasks
}, [])
}
68 changes: 30 additions & 38 deletions src/resolveTaskFn.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ const debug = require('debug')('lint-staged:task')
* return the promise.
*
* @param {string} cmd
* @param {Array<string>} args
* @param {Object} execaOptions
* @return {Promise} child_process
*/
const execLinter = (cmd, args, execaOptions = {}) => {
const execLinter = (cmd, args, execaOptions) => {
debug('cmd:', cmd)
if (args) debug('args:', args)
debug('execaOptions:', execaOptions)
Expand Down Expand Up @@ -75,52 +77,42 @@ function makeErr(linter, result, context = {}) {
* if the OS is Windows.
*
* @param {Object} options
* @param {string} options.gitDir
* @param {Boolean} options.isFn
* @param {string} options.linter
* @param {Boolean} options.shellMode
* @param {string} options.gitDir
* @param {Array<string>} options.pathsToLint
* @returns {function(): Promise<Array<string>>}
*/
module.exports = function resolveTaskFn({ gitDir, linter, pathsToLint, shell = false }) {
// If `linter` is a function, it should return a string when evaluated with `pathsToLint`.
// Else, it's a already a string
const fnLinter = typeof linter === 'function'
const linterString = fnLinter ? linter(pathsToLint) : linter
// Support arrays of strings/functions by treating everything as arrays
const linters = Array.isArray(linterString) ? linterString : [linterString]

module.exports = function resolveTaskFn({ gitDir, isFn, linter, pathsToLint, shell = false }) {
const execaOptions = { preferLocal: true, reject: false, shell }

const tasks = linters.map(command => {
let cmd
let args
let cmd
let args

if (shell) {
execaOptions.shell = true
// If `shell`, passed command shouldn't be parsed
// If `linter` is a function, command already includes `pathsToLint`.
cmd = fnLinter ? command : `${command} ${pathsToLint.join(' ')}`
} else {
const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(command)
cmd = parsedCmd
args = fnLinter ? parsedArgs : parsedArgs.concat(pathsToLint)
}

// Only use gitDir as CWD if we are using the git binary
// e.g `npm` should run tasks in the actual CWD
if (/^git(\.exe)?/i.test(command) && gitDir !== process.cwd()) {
execaOptions.cwd = gitDir
}
if (shell) {
execaOptions.shell = true
// If `shell`, passed command shouldn't be parsed
// If `linter` is a function, command already includes `pathsToLint`.
cmd = isFn ? linter : `${linter} ${pathsToLint.join(' ')}`
} else {
const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(linter)
cmd = parsedCmd
args = isFn ? parsedArgs : parsedArgs.concat(pathsToLint)
}

return ctx =>
execLinter(cmd, args, execaOptions).then(result => {
if (result.failed || result.killed || result.signal != null) {
throw makeErr(linter, result, ctx)
}
// Only use gitDir as CWD if we are using the git binary
// e.g `npm` should run tasks in the actual CWD
if (/^git(\.exe)?/i.test(linter) && gitDir !== process.cwd()) {
execaOptions.cwd = gitDir
}

return successMsg(linter)
})
})
return ctx =>
execLinter(cmd, args, execaOptions).then(result => {
if (result.failed || result.killed || result.signal != null) {
throw makeErr(linter, result, ctx)
}

return ctx => Promise.all(tasks.map(task => task(ctx)))
return successMsg(linter)
})
}
22 changes: 16 additions & 6 deletions test/makeCmdTasks.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,25 +61,35 @@ describe('makeCmdTasks', () => {

it('should work with function linter returning array of string', async () => {
const res = await makeCmdTasks(() => ['test', 'test2'], false, gitDir, ['test.js'])
expect(res.length).toBe(1)
expect(res.length).toBe(2)
expect(res[0].title).toEqual('test')
expect(res[1].title).toEqual('test2')
})

it('should work with function linter accepting arguments', async () => {
const res = await makeCmdTasks(
filenames => filenames.map(file => `test ${file}`),
false,
gitDir,
['test.js']
['test.js', 'test2.js']
)
expect(res.length).toBe(1)
expect(res[0].title).toEqual('test [file]')
expect(res.length).toBe(2)
expect(res[0].title).toEqual('test test.js')
expect(res[1].title).toEqual('test test2.js')
})

it('should work with array of mixed string and function linters', async () => {
const res = await makeCmdTasks([() => 'test', 'test2'], false, gitDir, ['test.js'])
expect(res.length).toBe(2)
const res = await makeCmdTasks(
[() => 'test', 'test2', files => files.map(file => `test ${file}`)],
false,
gitDir,
['test.js', 'test2.js', 'test3.js']
)
expect(res.length).toBe(5)
expect(res[0].title).toEqual('test')
expect(res[1].title).toEqual('test2')
expect(res[2].title).toEqual('test test.js')
expect(res[3].title).toEqual('test test2.js')
expect(res[4].title).toEqual('test test3.js')
})
})
36 changes: 25 additions & 11 deletions test/resolveTaskFn.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ describe('resolveTaskFn', () => {
})
})

it('should support function linters that return string', async () => {
it('should not append pathsToLint when isFn', async () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
linter: filenames => `node --arg=true ./myscript.js ${filenames}`
isFn: true,
linter: 'node --arg=true ./myscript.js test.js'
})

await taskFn()
Expand All @@ -40,25 +41,38 @@ describe('resolveTaskFn', () => {
})
})

it('should support function linters that return array of strings', async () => {
expect.assertions(3)
it('should not append pathsToLint when isFn and shell', async () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
pathsToLint: ['foo.js', 'bar.js'],
linter: filenames => filenames.map(filename => `node --arg=true ./myscript.js ${filename}`)
isFn: true,
shell: true,
linter: 'node --arg=true ./myscript.js test.js'
})

await taskFn()
expect(execa).toHaveBeenCalledTimes(2)
expect(execa).nthCalledWith(1, 'node', ['--arg=true', './myscript.js', 'foo.js'], {
expect(execa).toHaveBeenCalledTimes(1)
expect(execa).lastCalledWith('node --arg=true ./myscript.js test.js', {
preferLocal: true,
reject: false,
shell: false
shell: true
})
})

it('should work with shell', async () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
shell: true,
linter: 'node --arg=true ./myscript.js'
})
expect(execa).nthCalledWith(2, 'node', ['--arg=true', './myscript.js', 'bar.js'], {

await taskFn()
expect(execa).toHaveBeenCalledTimes(1)
expect(execa).lastCalledWith('node --arg=true ./myscript.js test.js', {
preferLocal: true,
reject: false,
shell: false
shell: true
})
})

Expand Down
13 changes: 5 additions & 8 deletions test/resolveTaskFn.unmocked.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,13 @@ describe('resolveTaskFn', () => {
it('should call execa with shell when configured so', async () => {
const taskFn = resolveTaskFn({
pathsToLint: ['package.json'],
linter: () => 'node -e "process.exit(1)" || echo $?',
isFn: true,
linter: 'node -e "process.exit(1)" || echo $?',
shell: true
})

await expect(taskFn()).resolves.toMatchInlineSnapshot(`
Array [
"√ function linter() {
return 'node -e \\"process.exit(1)\\" || echo $?';
} passed!",
]
`)
await expect(taskFn()).resolves.toMatchInlineSnapshot(
`"√ node -e \\"process.exit(1)\\" || echo $? passed!"`
)
})
})

0 comments on commit 0dd0c94

Please sign in to comment.