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/issue-1398: added support for custom configuration #1401
base: master
Are you sure you want to change the base?
Conversation
|
lib/makeCmdTasks.js
Outdated
'Function task should return a string or an array of strings', | ||
resolved | ||
if (isFn && command && typeof command === 'object') { | ||
if ( |
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 we flip the condition and do an early return?
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.
done
lib/makeCmdTasks.js
Outdated
}) | ||
} else { | ||
throw new Error( | ||
configurationError('[Function]', 'Function task should be in proper format', resolved) |
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 we provide a meaningful error message on what specifically was the problem?
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.
done
lib/makeCmdTasks.js
Outdated
} | ||
cmdTasks.push({ | ||
title: command.title, | ||
command: command.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.
Do we need both the command
and title
?
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 think we only need title, so removed the command field.
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 file has started to become a bit complex... I wonder if we could split it somehow, maybe different handling for function and regular config.
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.
done
We should add both unit and integration tests for this. |
Hey, please don't add versioning commits directly to the PR! It's automated in the GitHub Actions! I'm still thinking about this feature, there's no rush to get this in. |
iiroj I have incorporated all the review comments, you can proceed with the review/approval. |
This reverts commit eb8dd33.
While I appreciate your contribution, I don't think this feature is yet ready to be shipped with lint-staged and I want to spend some time thinking about it. For now I'll leave this PR open and use it as a base when I'm satisfied with the solution. I also want to think about deprecating the |
After thinking about this, I like the feature but also realized that we can probably simplify the config by one level without it being a breaking change: export default {
"**/*.{js,jsx}": {
title: "Running a custom task",
task: async (filenames) => {
console.time()
await doFoo(filenames)
console.timeEnd()
}
},
} With this method the config looks like: type Task = string
type CreateTask = (filenames: string[]) => Task | Task[] | Promise<Task | Task[]>
type FunctionTask = {
title: string
task: (filenames: string) => void | Promise<void>
}
type Configuration = Record<string, Task | Task[] | CreateTask | FunctionTask> Unless I'm mistaken... what do you think? After this change we should just add some integration tests with a real config. |
Fixture for #1398
By proposed changes user will get the opportunity to control/handle the execution of custom task, As you can see in following example
.lintstagedrc.js
.Following will be the output in case of no-console eslint error