-
-
Notifications
You must be signed in to change notification settings - Fork 425
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
Avoid stashing if no lint-staged files are changed (#570) #573
Avoid stashing if no lint-staged files are changed (#570) #573
Conversation
- add test to check if stashing is skipped if no lint-staged files are changed Fixes lint-staged#570
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
==========================================
+ Coverage 98.14% 98.14% +<.01%
==========================================
Files 13 13
Lines 377 378 +1
Branches 52 52
==========================================
+ Hits 370 371 +1
Misses 7 7
Continue to review full report at Codecov.
|
test/runAll.spec.js
Outdated
@@ -161,4 +161,31 @@ describe('runAll', () => { | |||
expect(err).toEqual('test') | |||
} | |||
}) | |||
|
|||
it.only('should skip stashing changes if no lint-staged files are changed', async () => { |
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.
Please remove only
to enable all tests again
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.
Whoops! I've fixed this in my next commit :)
src/runAll.js
Outdated
if (tasks.length) { | ||
let hasStagedTaskFiles = false | ||
tasks.forEach(task => { | ||
hasStagedTaskFiles = hasStagedTaskFiles || task.hasStagedFiles() |
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’m not sure I understand how this is working. Mind explaining in code comments?
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.
Changed the logic of this slightly in my next commit but also added a comment as you requested.
…ly" from "it.only" Fixes lint-staged#570
Looks like a test is failing: it('should resolve the promise with no files', async () => {
expect.assertions(1)
await runAll(getConfig({ linters: { '*.js': ['echo "sample"'] } }))
expect(console.printHistory()).toMatchSnapshot()
}) If I understand what this is checking for... I think this can be deleted now because the code path inside Alternatively, I could just update the snapshot to be "". What would you prefer? |
… no files" as stashing/linters are no longer executed if no staged files match the linters configuration
For the time being, I figured I'd just update the snapshot so that the tests pass. |
LOG → No staged files match *.js | ||
LOG Running linters... [completed]" | ||
`; | ||
exports[`runAll should resolve the promise with no files 1`] = `""`; |
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 doesn’t look correct to me. As a user I’d still like to get some feedback on why it’s not executed. The output here was correct and we should match it since both conditions are true. I think what you want to achieve is a skip
function on the stashing step that should check and return with “No staged files match any of provided globs” or similar. WYT?
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.
My logic was to skip executing any kind of command if no staged files matched the provided globs. If we prepend more tasks in the future like the one that stashes partial changes, I'd rather everything be guaranteed to be skipped.
But I'm happy to move the code there if you feel if it makes more sense.
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’m more concerned about the UX here as the silence could be frustrating for users. What do you think?
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 that's a fair point.
In other programming language communities, like Go, generally you only log if things were modified/changed. If nothing was processed or changed then nothing is logged. This is why I felt it was OK to log nothing.
In saying all this... I'm not sure this makes sense for a tool that is generally executed by humans not continuous integration machines. This behaviour might feel incorrect within the JavaScript community too.
So... maybe we can just log what you suggested and exit early so that the snapshot outputs:
LOG No linters executed. No staged files match any of provided globs in "lint-staged.linters"."
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.
That sounds good to me. So you still think we should not use Listr for this and exit earlier?
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 do.
My expectation is that the program would exit as soon as possible once it realizes it shouldn't process anything. Is that ok with you?
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.
Sounds good to me! Let’s do this
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.
Awesome :)
I'll most likely have time to get onto this in 2-3 days!
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've added this in and updated the snapshots accordingly!
src/runAll.js
Outdated
// avoid executing any "stashing changes..." logic | ||
let isSkippingAllLinters = true | ||
tasks.forEach(task => { | ||
if (!task.skip()) { |
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’m still wondering why do we need to execute a function defined on each task here. Also I’d prefer a more functional code style but consider this a nit pick
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.
Say I've got two rules for two different tasks/linters, ie. one for TypeScript and one for SASS files.
"lint-staged": {
"*.ts": ["tslint --fix", "git add"],
"*.scss": ["sass-lint --fix", "git add"]
}
I want to ensure I'm checking all tasks for file changes as somebody may be only committing TypeScript files or SASS files but not both.
So if only TypeScript files are staged but not SASS files, the first task will not skip but the second task will.
If SASS files are staged but not TypeScript files, the first task WILL skip but not the second.
Does that make sense?
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.
Yes I think I’ve got the logic. Let’s rewrite this in a more way so it matches the code style. What about using some
or every
of Array?
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.
Alright :)
I'm personally not a fan of functional programming paradigms applied to arrays, but I'll make the change regardless!
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 opted to use "every" as that made the most sense to me, readability wise :)
…ed is skipped and update snapshots Fixes lint-staged#570
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.
Almost there!
src/runAll.js
Outdated
// avoid executing any lint-staged logic | ||
if (tasks.every(task => task.skip())) { | ||
console.log( | ||
'No linters executed. No staged files match any of provided globs in "lint-staged.linters".' |
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.
Let’s fix grammar here and it’s good to merge. I’d say drop the No linters executed.
part. Also drop in “lint-staged.linters”
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.
Sure. I figured if we were going to report an error message, we'd make it clear precisely what we mean by "provided globs", config key-wise.
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've updated the copy :)
console.log( | ||
'No linters executed. No staged files match any of provided globs in "lint-staged.linters".' | ||
) | ||
return 'No tasks to run.' |
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’m wondering if we’re using this return value anywhere and if we should use return
for the other case.
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 was uncertain about this so I left it as is.
Let me know what you want to do. I feel it's best to probably just leave it unless you think it won't break anything. (Also is this function exported outside of the library? If so maybe someone has code relying on that string 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.
I’m not sure either. Let’s leave for now.
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.
Perfect! Thanks for working on it!
Thanks for working on this one! |
🎉 This PR is included in version 8.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #570