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.

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

Create shorter title for function tasks with many staged files #706

Merged
merged 3 commits into from Sep 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 21 additions & 12 deletions src/makeCmdTasks.js
Expand Up @@ -8,27 +8,36 @@ const debug = require('debug')('lint-staged:make-cmd-tasks')
* Creates and returns an array of listr tasks which map to the given commands.
*
* @param {object} options
* @param {Array<string|Function>|string|Function} [options.commands]
* @param {string} [options.gitDir]
* @param {Array<string>} [options.pathsToLint]
* @param {Array<string|Function>|string|Function} options.commands
* @param {Array<string>} options.files
* @param {string} options.gitDir
* @param {Boolean} shell
*/
module.exports = async function makeCmdTasks({ commands, gitDir, pathsToLint, shell }) {
module.exports = async function makeCmdTasks({ commands, files, gitDir, shell }) {
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`
// command function may return array of commands that already include `stagedFiles`
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 resolved = isFn ? command(files) : command
const commands = Array.isArray(resolved) ? resolved : [resolved] // Wrap non-array command as array

linters.forEach(linter => {
const task = {
title: linter,
task: resolveTaskFn({ gitDir, isFn, linter, pathsToLint, shell })
}
// Function command should not be used as the task title as-is
// because the resolved string it might be very long
// Create a matching command array with [file] in place of file names
let mockCommands
if (isFn) {
const mockFileList = Array(commands.length).fill('[file]')
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think I made an error here as simple as creating the mock file array from the length of the commands array, instead of the staged files array?

To have the mock file array match the actual staged files in length, this should be:

Suggested change
const mockFileList = Array(commands.length).fill('[file]')
const mockFileList = Array(files.length).fill('[file]')

const resolved = command(mockFileList)
mockCommands = Array.isArray(resolved) ? resolved : [resolved]
}

commands.forEach((command, i) => {
// If command is a function, use the matching mock command as title,
// but since might include multiple [file] arguments, shorten to one
const title = isFn ? mockCommands[i].replace(/\[file\].*\[file\]/, '[file]') : command
const task = { title, task: resolveTaskFn({ gitDir, isFn, command, files, shell }) }
tasks.push(task)
})

Expand Down
31 changes: 12 additions & 19 deletions src/resolveTaskFn.js
Expand Up @@ -77,27 +77,20 @@ function makeErr(linter, result, context = {}) {
* if the OS is Windows.
*
* @param {Object} options
* @param {String} [options.gitDir] - Current git repo path
* @param {Boolean} [options.isFn] - Whether the linter task is a function
* @param {string} [options.linter] — Linter task
* @param {Array<string>} [options.pathsToLint] — Filepaths to run the linter task against
* @param {string} options.command — Linter task
* @param {String} options.gitDir - Current git repo path
* @param {Boolean} options.isFn - Whether the linter task is a function
* @param {Array<string>} options.pathsToLint — Filepaths to run the linter task against
* @param {Boolean} [options.relative] — Whether the filepaths should be relative
* @param {Boolean} [options.shell] — Whether to skip parsing linter task for better shell support
* @returns {function(): Promise<Array<string>>}
*/
module.exports = function resolveTaskFn({
gitDir,
isFn,
linter,
pathsToLint,
relative,
shell = false
}) {
module.exports = function resolveTaskFn({ command, files, gitDir, isFn, relative, shell = false }) {
const execaOptions = { preferLocal: true, reject: false, shell }

if (relative) {
execaOptions.cwd = process.cwd()
} else if (/^git(\.exe)?/i.test(linter) && gitDir !== process.cwd()) {
} else if (/^git(\.exe)?/i.test(command) && gitDir !== process.cwd()) {
// Only use gitDir as CWD if we are using the git binary
// e.g `npm` should run tasks in the actual CWD
execaOptions.cwd = gitDir
Expand All @@ -109,20 +102,20 @@ module.exports = function resolveTaskFn({
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(' ')}`
// If `linter` is a function, command already includes `files`.
cmd = isFn ? command : `${command} ${files.join(' ')}`
} else {
const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(linter)
const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(command)
cmd = parsedCmd
args = isFn ? parsedArgs : parsedArgs.concat(pathsToLint)
args = isFn ? parsedArgs : parsedArgs.concat(files)
}

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

return successMsg(linter)
return successMsg(command)
})
}
2 changes: 1 addition & 1 deletion src/runAll.js
Expand Up @@ -73,7 +73,7 @@ https://github.com/okonet/lint-staged#using-js-functions-to-customize-linter-com
title: `Running tasks for ${task.pattern}`,
task: async () =>
new Listr(
await makeCmdTasks({ commands: task.commands, gitDir, shell, pathsToLint: task.fileList }),
await makeCmdTasks({ commands: task.commands, files: task.fileList, gitDir, shell }),
{
// In sub-tasks we don't want to run concurrently
// and we want to abort on errors
Expand Down
34 changes: 22 additions & 12 deletions test/makeCmdTasks.spec.js
Expand Up @@ -9,13 +9,13 @@ describe('makeCmdTasks', () => {
})

it('should return an array', async () => {
const array = await makeCmdTasks({ commands: 'test', gitDir, pathsToLint: ['test.js'] })
const array = await makeCmdTasks({ commands: 'test', gitDir, files: ['test.js'] })
expect(array).toBeInstanceOf(Array)
})

it('should work with a single command', async () => {
expect.assertions(4)
const res = await makeCmdTasks({ commands: 'test', gitDir, pathsToLint: ['test.js'] })
const res = await makeCmdTasks({ commands: 'test', gitDir, files: ['test.js'] })
expect(res.length).toBe(1)
const [linter] = res
expect(linter.title).toBe('test')
Expand All @@ -30,7 +30,7 @@ describe('makeCmdTasks', () => {
const res = await makeCmdTasks({
commands: ['test', 'test2'],
gitDir,
pathsToLint: ['test.js']
files: ['test.js']
})
expect(res.length).toBe(2)
const [linter1, linter2] = res
Expand Down Expand Up @@ -58,7 +58,7 @@ describe('makeCmdTasks', () => {
})

it('should work with function linter returning a string', async () => {
const res = await makeCmdTasks({ commands: () => 'test', gitDir, pathsToLint: ['test.js'] })
const res = await makeCmdTasks({ commands: () => 'test', gitDir, files: ['test.js'] })
expect(res.length).toBe(1)
expect(res[0].title).toEqual('test')
})
Expand All @@ -67,7 +67,7 @@ describe('makeCmdTasks', () => {
const res = await makeCmdTasks({
commands: () => ['test', 'test2'],
gitDir,
pathsToLint: ['test.js']
files: ['test.js']
})
expect(res.length).toBe(2)
expect(res[0].title).toEqual('test')
Expand All @@ -78,24 +78,34 @@ describe('makeCmdTasks', () => {
const res = await makeCmdTasks({
commands: filenames => filenames.map(file => `test ${file}`),
gitDir,
pathsToLint: ['test.js', 'test2.js']
files: ['test.js', 'test2.js']
})
expect(res.length).toBe(2)
expect(res[0].title).toEqual('test test.js')
expect(res[1].title).toEqual('test test2.js')
expect(res[0].title).toEqual('test [file]')
expect(res[1].title).toEqual('test [file]')
})

it('should work with array of mixed string and function linters', async () => {
const res = await makeCmdTasks({
commands: [() => 'test', 'test2', files => files.map(file => `test ${file}`)],
gitDir,
pathsToLint: ['test.js', 'test2.js', 'test3.js']
files: ['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')
expect(res[2].title).toEqual('test [file]')
expect(res[3].title).toEqual('test [file]')
expect(res[4].title).toEqual('test [file]')
})

it('should generate short names for function tasks with long file list', async () => {
const res = await makeCmdTasks({
commands: filenames => `test ${filenames.map(file => `--file ${file}`).join(' ')}`,
gitDir,
files: Array(100).fill('file.js') // 100 times `file.js`
})
expect(res.length).toBe(1)
expect(res[0].title).toEqual('test --file [file]')
})
})
24 changes: 12 additions & 12 deletions test/resolveTaskFn.spec.js
@@ -1,7 +1,7 @@
import execa from 'execa'
import resolveTaskFn from '../src/resolveTaskFn'

const defaultOpts = { pathsToLint: ['test.js'] }
const defaultOpts = { files: ['test.js'] }

describe('resolveTaskFn', () => {
beforeEach(() => {
Expand All @@ -12,7 +12,7 @@ describe('resolveTaskFn', () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
linter: 'node --arg=true ./myscript.js'
command: 'node --arg=true ./myscript.js'
})

await taskFn()
Expand All @@ -29,7 +29,7 @@ describe('resolveTaskFn', () => {
const taskFn = resolveTaskFn({
...defaultOpts,
isFn: true,
linter: 'node --arg=true ./myscript.js test.js'
command: 'node --arg=true ./myscript.js test.js'
})

await taskFn()
Expand All @@ -47,7 +47,7 @@ describe('resolveTaskFn', () => {
...defaultOpts,
isFn: true,
shell: true,
linter: 'node --arg=true ./myscript.js test.js'
command: 'node --arg=true ./myscript.js test.js'
})

await taskFn()
Expand All @@ -64,7 +64,7 @@ describe('resolveTaskFn', () => {
const taskFn = resolveTaskFn({
...defaultOpts,
shell: true,
linter: 'node --arg=true ./myscript.js'
command: 'node --arg=true ./myscript.js'
})

await taskFn()
Expand All @@ -80,7 +80,7 @@ describe('resolveTaskFn', () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
linter: 'git add',
command: 'git add',
gitDir: '../'
})

Expand All @@ -96,7 +96,7 @@ describe('resolveTaskFn', () => {

it('should not pass `gitDir` as `cwd` to `execa()` if a non-git binary is called', async () => {
expect.assertions(2)
const taskFn = resolveTaskFn({ ...defaultOpts, linter: 'jest', gitDir: '../' })
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'jest', gitDir: '../' })

await taskFn()
expect(execa).toHaveBeenCalledTimes(1)
Expand All @@ -111,7 +111,7 @@ describe('resolveTaskFn', () => {
expect.assertions(2)
const taskFn = resolveTaskFn({
...defaultOpts,
linter: 'git add',
command: 'git add',
relative: true
})

Expand All @@ -135,7 +135,7 @@ describe('resolveTaskFn', () => {
cmd: 'mock cmd'
})

const taskFn = resolveTaskFn({ ...defaultOpts, linter: 'mock-fail-linter' })
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'mock-fail-linter' })
try {
await taskFn()
} catch (err) {
Expand All @@ -161,7 +161,7 @@ Mock error"
cmd: 'mock cmd'
})

const taskFn = resolveTaskFn({ ...defaultOpts, linter: 'mock-killed-linter' })
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'mock-killed-linter' })
try {
await taskFn()
} catch (err) {
Expand All @@ -177,7 +177,7 @@ Mock error"
it('should not set hasErrors on context if no error occur', async () => {
expect.assertions(1)
const context = {}
const taskFn = resolveTaskFn({ ...defaultOpts, linter: 'jest', gitDir: '../' })
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'jest', gitDir: '../' })
await taskFn(context)
expect(context.hasErrors).toBeUndefined()
})
Expand All @@ -191,7 +191,7 @@ Mock error"
cmd: 'mock cmd'
})
const context = {}
const taskFn = resolveTaskFn({ ...defaultOpts, linter: 'mock-fail-linter' })
const taskFn = resolveTaskFn({ ...defaultOpts, command: 'mock-fail-linter' })
expect.assertions(1)
try {
await taskFn(context)
Expand Down
4 changes: 2 additions & 2 deletions test/resolveTaskFn.unmocked.spec.js
Expand Up @@ -5,9 +5,9 @@ jest.unmock('execa')
describe('resolveTaskFn', () => {
it('should call execa with shell when configured so', async () => {
const taskFn = resolveTaskFn({
pathsToLint: ['package.json'],
command: 'node -e "process.exit(1)" || echo $?',
files: ['package.json'],
isFn: true,
linter: 'node -e "process.exit(1)" || echo $?',
shell: true
})

Expand Down
26 changes: 6 additions & 20 deletions test/runAll.unmocked.spec.js
Expand Up @@ -172,16 +172,9 @@ describe('runAll', () => {
expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.replace(/\n$/, ''))

// Since edit was not staged, the file is still modified
expect(await execGit(['status'])).toMatchInlineSnapshot(`
"On branch master
Changes not staged for commit:
(use \\"git add <file>...\\" to update what will be committed)
(use \\"git checkout -- <file>...\\" to discard changes in working directory)

modified: test.js

no changes added to commit (use \\"git add\\" and/or \\"git commit -a\\")"
`)
const status = await execGit(['status'])
expect(status).toMatch('modified: test.js')
expect(status).toMatch('no changes added to commit')
expect(await readFile('test.js')).toEqual(testJsFilePretty + appended)
})

Expand Down Expand Up @@ -210,16 +203,9 @@ no changes added to commit (use \\"git add\\" and/or \\"git commit -a\\")"
expect(await execGit(['show', 'HEAD:test.js'])).toEqual(testJsFilePretty.replace(/\n$/, ''))

// Nothing is staged
expect(await execGit(['status'])).toMatchInlineSnapshot(`
"On branch master
Changes not staged for commit:
(use \\"git add <file>...\\" to update what will be committed)
(use \\"git checkout -- <file>...\\" to discard changes in working directory)

modified: test.js

no changes added to commit (use \\"git add\\" and/or \\"git commit -a\\")"
`)
const status = await execGit(['status'])
expect(status).toMatch('modified: test.js')
expect(status).toMatch('no changes added to commit')

// File is pretty, and has been edited
expect(await readFile('test.js')).toEqual(testJsFilePretty + appended)
Expand Down