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

silbinarywolf · 2019-01-24T04:46:11Z

add check for lint-staged files before stashing changes
add test to check if stashing is skipped if no lint-staged files are changed

Fixes #570

- add test to check if stashing is skipped if no lint-staged files are changed Fixes lint-staged#570

codecov · 2019-01-24T04:52:06Z

Codecov Report

Merging #573 into master will increase coverage by <.01%.
The diff coverage is 100%.

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

Impacted Files	Coverage Δ
src/runAll.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e738d...162a7ff. Read the comment docs.

okonet · 2019-01-24T08:50:01Z

test/runAll.spec.js

@@ -161,4 +161,31 @@ describe('runAll', () => {
      expect(err).toEqual('test')
    }
  })
+
+  it.only('should skip stashing changes if no lint-staged files are changed', async () => {


Please remove only to enable all tests again

Whoops! I've fixed this in my next commit :)

okonet · 2019-01-24T08:52:13Z

src/runAll.js

-    if (tasks.length) {
+    let hasStagedTaskFiles = false
+    tasks.forEach(task => {
+      hasStagedTaskFiles = hasStagedTaskFiles || task.hasStagedFiles()


I’m not sure I understand how this is working. Mind explaining in code comments?

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

silbinarywolf · 2019-01-25T00:08:39Z

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 if (!isSkippingAllLinters) is no longer executed in that test case, it just returns immediately.

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

silbinarywolf · 2019-01-25T00:16:12Z

For the time being, I figured I'd just update the snapshot so that the tests pass.

okonet · 2019-01-25T08:53:14Z

test/__snapshots__/runAll.spec.js.snap

-LOG → No staged files match *.js
-LOG Running linters... [completed]"
-`;
+exports[`runAll should resolve the promise with no files 1`] = `""`;


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?

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.

I’m more concerned about the UX here as the silence could be frustrating for users. What do you think?

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

That sounds good to me. So you still think we should not use Listr for this and exit earlier?

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?

Sounds good to me! Let’s do this

Awesome :)
I'll most likely have time to get onto this in 2-3 days!

I've added this in and updated the snapshots accordingly!

okonet · 2019-01-25T08:54:30Z

src/runAll.js

+    // avoid executing any "stashing changes..." logic
+    let isSkippingAllLinters = true
+    tasks.forEach(task => {
+      if (!task.skip()) {


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

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?

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?

Alright :)
I'm personally not a fan of functional programming paradigms applied to arrays, but I'll make the change regardless!

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

okonet

Almost there!

okonet · 2019-01-29T06:40:33Z

src/runAll.js

+    // 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".'


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”

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.

I've updated the copy :)

okonet · 2019-01-29T06:41:20Z

src/runAll.js

+      console.log(
+        'No linters executed. No staged files match any of provided globs in "lint-staged.linters".'
+      )
+      return 'No tasks to run.'


I’m wondering if we’re using this return value anywhere and if we should use return for the other case.

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

I’m not sure either. Let’s leave for now.

okonet

Perfect! Thanks for working on it!

…iles

okonet · 2019-02-02T08:54:51Z

Thanks for working on this one!

okonet · 2019-02-02T08:57:08Z

🎉 This PR is included in version 8.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: add check for changed lint-staged files before stashing changes

04aa58f

- add test to check if stashing is skipped if no lint-staged files are changed Fixes lint-staged#570

okonet requested changes Jan 24, 2019

View reviewed changes

fix: simplify implementation, add comment as requested and remove "on…

ec66acd

…ly" from "it.only" Fixes lint-staged#570

fix: update test snapshot for "runAll should resolve the promise with…

316a7ed

… no files" as stashing/linters are no longer executed if no staged files match the linters configuration

okonet requested changes Jan 25, 2019

View reviewed changes

fix: replace Array.forEach with Array.every, add message if lint-stag…

a106320

…ed is skipped and update snapshots Fixes lint-staged#570

okonet requested changes Jan 29, 2019

View reviewed changes

fix: remove "No linters executed" and "in lint-staged.linters" copy

e7805f3

okonet approved these changes Jan 30, 2019

View reviewed changes

Merge branch 'master' into fix/github-issue-570-stashing-non-linted-f…

162a7ff

…iles

silbinarywolf mentioned this pull request Feb 2, 2019

git commit fails, ultimately causing lost work #565

Closed

okonet merged commit 8c4d9c9 into lint-staged:master Feb 2, 2019

okonet added the released label Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

silbinarywolf commented Jan 24, 2019

codecov bot commented Jan 24, 2019 •

edited

okonet Jan 24, 2019

silbinarywolf Jan 25, 2019

okonet Jan 24, 2019

silbinarywolf Jan 25, 2019

silbinarywolf commented Jan 25, 2019 •

edited

silbinarywolf commented Jan 25, 2019

okonet Jan 25, 2019

silbinarywolf Jan 25, 2019

okonet Jan 25, 2019

silbinarywolf Jan 26, 2019

okonet Jan 26, 2019

silbinarywolf Jan 26, 2019

okonet Jan 26, 2019

silbinarywolf Jan 27, 2019

silbinarywolf Jan 29, 2019

okonet Jan 25, 2019

silbinarywolf Jan 25, 2019 •

edited

okonet Jan 25, 2019

silbinarywolf Jan 26, 2019

silbinarywolf Jan 29, 2019

okonet left a comment

okonet Jan 29, 2019

silbinarywolf Jan 29, 2019

silbinarywolf Jan 29, 2019

okonet Jan 29, 2019

silbinarywolf Jan 29, 2019

okonet Jan 29, 2019

okonet left a comment

okonet commented Feb 2, 2019

okonet commented Feb 2, 2019

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

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

Conversation

silbinarywolf commented Jan 24, 2019

codecov bot commented Jan 24, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silbinarywolf commented Jan 25, 2019 • edited

silbinarywolf commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silbinarywolf Jan 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet left a comment

Choose a reason for hiding this comment

okonet commented Feb 2, 2019

okonet commented Feb 2, 2019

codecov bot commented Jan 24, 2019 •

edited

silbinarywolf commented Jan 25, 2019 •

edited

silbinarywolf Jan 25, 2019 •

edited