lint-staged: preserve linter fixes after precommit hook fails

[OnkarRuikar]

Addresses https://github.com/orgs/mdn/discussions/428#discussioncomment-6537591

[bsmth]

Thanks Onkar, what are the downsides of this? From the docs:

--no-stash: By default a backup stash will be created before running the tasks, 
and all task modifications will be reverted in case of an error. 
This option will disable creating the stash, and instead leave all 
modifications in the index when aborting the commit. 
Can be re-enabled with --stash.

https://github.com/okonet/lint-staged#command-line-flags

[OnkarRuikar]

Thanks Onkar, what are the downsides of this? From the docs:

In this repo we are running only linters and an image optimizer. I don't see any case where authors would want their unlinted/unoptimized content back. 😁

To be safe it would be nice if you ping @mdn/core-dev team.

[bsmth]

I'm going to leave a +1 but it would be great if @hamishwillee can have a try to see if it resolves the issue (I'm not sure if you can reproduce the error).

[hamishwillee]

@OnkarRuikar The formatting was fixed up by the hook, but I still get an error:

[STARTED] Preparing lint-staged...
[SUCCESS] Preparing lint-staged...
[STARTED] Running tasks for staged files...
[STARTED] .lintstagedrc.json — 1 file
[STARTED] * — 1 file
[STARTED] *.md — 1 file
[STARTED] tests/**/*.* — 0 files
[STARTED] *.{svg,png,jpeg,jpg,gif} — 0 files
[SKIPPED] tests/**/*.* — no files
[SKIPPED] *.{svg,png,jpeg,jpg,gif} — no files
[STARTED] prettier --ignore-unknown --write
[STARTED] markdownlint-cli2-fix
[SUCCESS] markdownlint-cli2-fix
[STARTED] node scripts/front-matter_linter.js --fix true
[SUCCESS] prettier --ignore-unknown --write
[SUCCESS] * — 1 file
[SUCCESS] node scripts/front-matter_linter.js --fix true
[SUCCESS] *.md — 1 file
[SUCCESS] .lintstagedrc.json — 1 file
[SUCCESS] Running tasks for staged files...
[STARTED] Applying modifications from tasks...
[FAILED] Prevented an empty git commit!

  ⚠ lint-staged prevented an empty git commit.
  Use the --allow-empty option to continue, or check your task configuration

husky - pre-commit hook exited with code 1 (error)

[OnkarRuikar]

but I still get an error:

It is expected. Why do you want to make a commit(empty) that doesn't have any changes?
If you want to make an empty commit to trigger workflows or tag a release then use --no-verify option:

git commit --no-verify -m "message"
git commit -n -m "message"

This will skip all the hooks.

If this isn't the case then please provide the file name and the changes you are making.

[hamishwillee]

I'm an idiot - I was changing a file to see if it would prettify, but not realizing that after prettifying the file would return to its state before committing.

I tested this again by copying files\en-us\web\api\attr\prefix\index.html , modifying the javascript in both the old file and new file, then committing. This worked for the new file (though of course the old file was unchanged)

Looks good to me, but obviously this proves perhaps I'm not the right reviewer :-(

[OnkarRuikar]

I was changing a file to see if it would prettify, but not realizing that after prettifying the file would return to its state before committing.

Still this fix saves us, in this case, from manually reverting the changes after precommit fails. So there is no harm in merging this.

[OnkarRuikar]

@LeoMcA could you review and if required merge this?

[LeoMcA]

I'm not sure that this is a good idea, here's why:

• In the best case, when lint-staged makes changes, and raises no errors, the behaviour is the same before/after, so that's fine
• In the case when lint-staged makes changes, but also raises an error requiring manual intervention:
  • Before: it would revert the changes it made - in a lot of cases this shouldn't matter, whatever change is required should be able to be done in isolation compared to whatever other changes prettier/markdownlint are making, but even if it's not, one can manually run npx lint-staged --no-stash or yarn fix:md or whatever to make the changes again
  • After: we keep the changes, this is also fine, it saves manually running the linter
• In the worst case, where lint-staged fails in a way which is destructive:
  • Before: it's all good, it reverts the changes it made, we're back to where we were
  • After: uh oh - we've lost content, and it's unlikely to be stored anywhere (git reflog won't help if it hasn't been committed), there's no way back to the previous state

Since this change in the best case saves running a command, but in the worst case loses edits, I don't think it's a change we should make.

Happy to be convinced otherwise if I've missed something.

[OnkarRuikar]

Closing this considering the above comment.
It's always safe to manually run the linters afterwards than loosing the changes in unexpected events.

[bsmth]

It's always safe to manually run the linters afterwards than loosing the changes in unexpected events.

Seems like the best option. Thanks @LeoMcA for the 2¢