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

Jump to bottom

feat: improve task naming #834

Closed
wants to merge 2 commits into from
Closed

feat: improve task naming #834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
655dbea
feat: improve task naming
jd20 Apr 5, 2020
413e570
Merge branch 'master' into improve-task-naming
jd20 Apr 5, 2020
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
43 changes: 30 additions & 13 deletions lib/makeCmdTasks.js
View file
Open in desktop
Expand Up @@ -27,25 +27,42 @@ module.exports = async function makeCmdTasks({ commands, files, gitDir, shell })
const resolvedArray = Array.isArray(resolved) ? resolved : [resolved] // Wrap non-array command as array

for (const command of resolvedArray) {
let title = isFn ? '[Function]' : command
let title = isFn ? '[Function] ' : ''

if (isFn) {
// If the function linter didn't return string | string[] it won't work
// Do the validation here instead of `validateConfig` to skip evaluating the function multiple times
if (typeof command !== 'string') {
throw new Error(
createError(
title,
'Function task should return a string or an array of strings',
resolved
)
// If the function linter didn't return string | string[] it won't work
// Do the validation here instead of `validateConfig` to skip evaluating the function multiple times
if (isFn && typeof command !== 'string') {
throw new Error(
createError(
title.trim(),
'Function task should return a string or an array of strings',
resolved
)
)
}

const words = command.trim().split(' ') // Tokenize the command into words

if (isFn) {
const indexOfFile = words.findIndex(word => files.includes(word)) // index of first file in the command
Copy link
Member

Choose a reason for hiding this comment

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

I fear this might not work as-is, since the task function can format files.

It might be necessary to do something like:

const fileBasenames = files.map(file => path.basename(file))
const indexOfFile = words.findIndex(word => fileBasenames.some(basename => word.includes(basename)))

to match only the basename (ie. file.js).

This is bound to have performance indications, however.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I also realized tokenizing by splitting on space isn't going to work properly either, if there are spaces inside the filename. Let me revise it a bit more, to try and handle the basename logic above. Is the function formatting files the reason Windows test is failing as well?

Copy link
Member

Choose a reason for hiding this comment

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

Can’t say for sure, though I would think the files available in this method are normalized the same way; apparently not.

if (indexOfFile >= 0) {
words.splice(indexOfFile) // Strip staged files from command
}
}

const [startOfFn] = command.split(' ')
title += ` ${startOfFn} ...` // Append function name, like `[Function] eslint ...`
// For the title, strip package runner commands (npm/npx/yarn) from the beginning
if (words.length > 0 && ['npm', 'npx', 'yarn'].includes(words[0])) {
let idx = 1
while (idx < words.length && (words[idx] === '' || words[idx].startsWith('-'))) {
idx += 1 // Remove any options to the package runner command
}
if (idx < words.length) {
words.splice(0, idx) // Make sure we don't strip the entire command
}
Comment on lines +53 to +61
Copy link
Member

@iiroj iiroj Apr 6, 2020 •
edited

Choose a reason for hiding this comment

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

Not sure about this. How will it format something like

(files) => `npm run test -- --update ${files.join(' ')}`

Copy link
Author

Choose a reason for hiding this comment

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

It'll format it as run test -- --update. Do you think it makes sense to special-case the run command? Or maybe in general it is going to be too difficult to properly handle package runner commands, without overcomplicating things?

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s best to leave out stripping those. I’d imagine there’s always some reason to run via npm/yarn instead of directly, so better not hide it.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll update the PR.

}

title += words.join(' ') // Append formatted command, like `[Function] eslint`

cmdTasks.push({
title,
command,
Expand Down
47 changes: 37 additions & 10 deletions test/makeCmdTasks.spec.js
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ describe('makeCmdTasks', () => {
it('should work with function task returning a string', async () => {
const res = await makeCmdTasks({ commands: () => 'test', gitDir, files: ['test.js'] })
expect(res.length).toBe(1)
expect(res[0].title).toEqual('[Function] test ...')
expect(res[0].title).toEqual('[Function] test')
})

it('should work with function task returning array of string', async () => {
Expand All @@ -71,8 +71,8 @@ describe('makeCmdTasks', () => {
files: ['test.js']
})
expect(res.length).toBe(2)
expect(res[0].title).toEqual('[Function] test ...')
expect(res[1].title).toEqual('[Function] test2 ...')
expect(res[0].title).toEqual('[Function] test')
expect(res[1].title).toEqual('[Function] test2')
})

it('should work with function task accepting arguments', async () => {
Expand All @@ -82,8 +82,35 @@ describe('makeCmdTasks', () => {
files: ['test.js', 'test2.js']
})
expect(res.length).toBe(2)
expect(res[0].title).toEqual('[Function] test ...')
expect(res[1].title).toEqual('[Function] test ...')
expect(res[0].title).toEqual('[Function] test')
expect(res[1].title).toEqual('[Function] test')
})

it('should strip arguments regardless of order', async () => {
const res = await makeCmdTasks({
commands: filenames => `test ${filenames.reverse().join(' ')}`,
gitDir,
files: ['test.js', 'test2.js']
})
expect(res.length).toBe(1)
expect(res[0].title).toEqual('[Function] test')
})

it('should strip package runner commands', async () => {
const res = await makeCmdTasks({
commands: [
filenames => `yarn test -o --watch ${filenames[0]}`,
filenames => `npx --no-install test ${filenames[0]}`,
filenames => `npm --version ${filenames[0]}`
],
gitDir,
files: ['test.js']
})
const [cmd1, cmd2, cmd3] = res

expect(cmd1.title).toEqual('[Function] test -o --watch')
expect(cmd2.title).toEqual('[Function] test')
expect(cmd3.title).toEqual('[Function] npm --version')
})

it('should work with array of mixed string and function tasks', async () => {
Expand All @@ -93,17 +120,17 @@ describe('makeCmdTasks', () => {
files: ['test.js', 'test2.js', 'test3.js']
})
expect(res.length).toBe(5)
expect(res[0].title).toEqual('[Function] test ...')
expect(res[0].title).toEqual('[Function] test')
expect(res[1].title).toEqual('test2')
expect(res[2].title).toEqual('[Function] test ...')
expect(res[3].title).toEqual('[Function] test ...')
expect(res[4].title).toEqual('[Function] test ...')
expect(res[2].title).toEqual('[Function] test')
expect(res[3].title).toEqual('[Function] test')
expect(res[4].title).toEqual('[Function] test')
})

it('should work with async function tasks', async () => {
const res = await makeCmdTasks({ commands: async () => 'test', gitDir, files: ['test.js'] })
expect(res.length).toBe(1)
expect(res[0].title).toEqual('[Function] test ...')
expect(res[0].title).toEqual('[Function] test')
})

it("should throw when function task doesn't return string | string[]", async () => {
Expand Down
8 changes: 4 additions & 4 deletions test/runAll.unmocked.spec.js
View file
Open in desktop
Expand Up @@ -770,8 +770,8 @@ describe('runAll', () => {
LOG Preparing... [completed]
LOG Running tasks... [started]
LOG Running tasks for *.js [started]
LOG [Function] git ... [started]
LOG [Function] git ... [completed]
LOG [Function] git stash drop [started]
LOG [Function] git stash drop [completed]
LOG Running tasks for *.js [completed]
LOG Running tasks... [completed]
LOG Applying modifications... [started]
Expand Down Expand Up @@ -992,8 +992,8 @@ describe('runAll', () => {
LOG Hiding unstaged changes to partially staged files... [completed]
LOG Running tasks... [started]
LOG Running tasks for *.js [started]
LOG [Function] prettier ... [started]
LOG [Function] prettier ... [completed]
LOG [Function] prettier --write [started]
LOG [Function] prettier --write [completed]
LOG Running tasks for *.js [completed]
LOG Running tasks... [completed]
LOG Applying modifications... [started]
Expand Down