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 3 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
11 changes: 10 additions & 1 deletion src/runAll.js
View file
Open in desktop
Expand Up @@ -62,7 +62,16 @@ module.exports = function runAll(config) {
renderer
}

if (tasks.length) {
// If all of the configured "linters" should be skipped
// avoid executing any "stashing changes..." logic
let isSkippingAllLinters = true
tasks.forEach(task => {
if (!task.skip()) {
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 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

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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!

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 opted to use "every" as that made the most sense to me, readability wise :)

isSkippingAllLinters = false
}
})

if (!isSkippingAllLinters) {
// Do not terminate main Listr process on SIGINT
process.on('SIGINT', () => {})

Expand Down
14 changes: 3 additions & 11 deletions test/__snapshots__/runAll.spec.js.snap
View file
Open in desktop
Expand Up @@ -29,17 +29,7 @@ LOG Running tasks for *.js [completed]
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]"
`;
exports[`runAll should resolve the promise with no files 1`] = `""`;
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

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 more concerned about the UX here as the silence could be frustrating for users. What do you think?

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 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"."

Copy link
Collaborator

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?

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 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?

Copy link
Collaborator

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

Copy link
Contributor Author

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!

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 added this in and updated the snapshots accordingly!


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

exports[`runAll should skip stashing changes if no lint-staged files are changed 1`] = `""`;

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