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
feat: improve task naming #834
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(' ')}` There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'll format it as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
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 fear this might not work as-is, since the task function can format files.
It might be necessary to do something like:
to match only the basename (ie.
file.js
).This is bound to have performance indications, however.
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.
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?
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.
Can’t say for sure, though I would think the
files
available in this method are normalized the same way; apparently not.