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

Avoid stashing if no lint-staged files are changed (#570) #573

Merged
merged 6 commits into from Feb 2, 2019
Merged

Avoid stashing if no lint-staged files are changed (#570) #573

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04aa58f
fix: add check for changed lint-staged files before stashing changes
silbinarywolf Jan 24, 2019
ec66acd
fix: simplify implementation, add comment as requested and remove "on…
silbinarywolf Jan 25, 2019
316a7ed
fix: update test snapshot for "runAll should resolve the promise with…
silbinarywolf Jan 25, 2019
a106320
fix: replace Array.forEach with Array.every, add message if lint-stag…
silbinarywolf Jan 29, 2019
e7805f3
fix: remove "No linters executed" and "in lint-staged.linters" copy
silbinarywolf Jan 29, 2019
162a7ff
Merge branch 'master' into fix/github-issue-570-stashing-non-linted-f…
okonet Jan 30, 2019
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
89 changes: 47 additions & 42 deletions src/runAll.js
View file
Open in desktop
Expand Up @@ -62,51 +62,56 @@ module.exports = function runAll(config) {
renderer
}

if (tasks.length) {
// Do not terminate main Listr process on SIGINT
process.on('SIGINT', () => {})
// If all of the configured "linters" should be skipped
// 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".'
Copy link
Collaborator

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”

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 :)

)
return 'No tasks to run.'
Copy link
Collaborator

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.

Copy link
Contributor Author

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!)

Copy link
Collaborator

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.

}

return new Listr(
[
{
title: 'Stashing changes...',
skip: async () => {
const hasPSF = await git.hasPartiallyStagedFiles()
if (!hasPSF) {
return 'No partially staged files found...'
}
return false
},
task: ctx => {
ctx.hasStash = true
return git.gitStashSave()
// Do not terminate main Listr process on SIGINT
process.on('SIGINT', () => {})

return new Listr(
[
{
title: 'Stashing changes...',
skip: async () => {
const hasPSF = await git.hasPartiallyStagedFiles()
if (!hasPSF) {
return 'No partially staged files found...'
}
return false
},
{
title: 'Running linters...',
task: () =>
new Listr(tasks, {
...listrBaseOptions,
concurrent,
exitOnError: !concurrent // Wait for all errors when running concurrently
})
},
{
title: 'Updating stash...',
enabled: ctx => ctx.hasStash,
skip: ctx =>
ctx.hasErrors && 'Skipping stash update since some tasks exited with errors',
task: () => git.updateStash()
},
{
title: 'Restoring local changes...',
enabled: ctx => ctx.hasStash,
task: () => git.gitStashPop()
task: ctx => {
ctx.hasStash = true
return git.gitStashSave()
}
],
listrBaseOptions
).run()
}
return 'No tasks to run.'
},
{
title: 'Running linters...',
task: () =>
new Listr(tasks, {
...listrBaseOptions,
concurrent,
exitOnError: !concurrent // Wait for all errors when running concurrently
})
},
{
title: 'Updating stash...',
enabled: ctx => ctx.hasStash,
skip: ctx => ctx.hasErrors && 'Skipping stash update since some tasks exited with errors',
task: () => git.updateStash()
},
{
title: 'Restoring local changes...',
enabled: ctx => ctx.hasStash,
task: () => git.gitStashPop()
}
],
listrBaseOptions
).run()
})
}
43 changes: 35 additions & 8 deletions test/__snapshots__/runAll.spec.js.snap
View file
Open in desktop
Expand Up @@ -31,14 +31,7 @@ LOG Running linters... [completed]"

exports[`runAll should resolve the promise with no files 1`] = `
"
LOG Stashing changes... [started]
LOG Stashing changes... [skipped]
LOG → No partially staged files found...
LOG Running linters... [started]
LOG Running tasks for *.js [started]
LOG Running tasks for *.js [skipped]
LOG → No staged files match *.js
LOG Running linters... [completed]"
LOG No linters executed. No staged files match any of provided globs in \\"lint-staged.linters\\"."
`;

exports[`runAll should skip linters and stash update but perform working copy restore if terminated 1`] = `
Expand Down Expand Up @@ -83,6 +76,40 @@ LOG Running tasks for *.js [completed]
LOG Running linters... [completed]"
`;

exports[`runAll should skip stashing changes if no lint-staged files are changed 1`] = `
"
LOG No linters executed. No staged files match any of provided globs in \\"lint-staged.linters\\"."
`;

exports[`runAll should skip updating stash if there are errors during linting 1`] = `
"
LOG Stashing changes... [started]
LOG Stashing changes... [completed]
LOG Running linters... [started]
LOG Running tasks for *.js [started]
LOG echo \\"sample\\" [started]
LOG echo \\"sample\\" [failed]
LOG →
LOG Running tasks for *.js [failed]
LOG →
LOG Running linters... [failed]
LOG Updating stash... [started]
LOG Updating stash... [skipped]
LOG → Skipping stash update since some tasks exited with errors
LOG Restoring local changes... [started]
LOG Restoring local changes... [completed]
LOG {
name: 'ListrError',
errors: [
{
privateMsg: '\\\\n\\\\n\\\\n× echo \\"sample\\" found some errors. Please fix them and try committing again.\\\\n\\\\nLinter finished with error',
context: {hasStash: true, hasErrors: true}
}
],
context: {hasStash: true, hasErrors: true}
}"
`;

exports[`runAll should skip updating stash if there are errors during linting 1`] = `
"
LOG Stashing changes... [started]
Expand Down
27 changes: 27 additions & 0 deletions test/runAll.spec.js
View file
Open in desktop
Expand Up @@ -161,4 +161,31 @@ describe('runAll', () => {
expect(err).toEqual('test')
}
})

it('should skip stashing changes if no lint-staged files are changed', async () => {
expect.assertions(4)
hasPartiallyStagedFiles.mockImplementationOnce(() => Promise.resolve(true))
sgfMock.mockImplementationOnce((params, callback) => {
callback(null, [{ filename: 'sample.java', status: 'Modified' }])
})
execa.mockImplementationOnce(() =>
Promise.resolve({
stdout: '',
stderr: 'Linter finished with error',
code: 1,
failed: true,
cmd: 'mock cmd'
})
)

try {
await runAll(getConfig({ linters: { '*.js': ['echo "sample"'] } }))
} catch (err) {
console.log(err)
}
expect(console.printHistory()).toMatchSnapshot()
expect(gitStashSave).toHaveBeenCalledTimes(0)
expect(updateStash).toHaveBeenCalledTimes(0)
expect(gitStashPop).toHaveBeenCalledTimes(0)
})
})