Lint-staged: "Bailing": Failing when `prettier --write` (or similar) fixes a file? (No "git add" task)

[jorgeorpinel]

Description

I'm trying to replace pretty-quick (automated with a Git pre-commit hook) with this tool. The former has the --bail behavior which allows me to review any changes done by Prettier by using git diff, before adding them myself (so no "git add" task in lint-staged config) and trying to commit again. I haven't found a way to set this up 🙁 Is it possible?

Environment

• OS: macOS Catalina
• Node.js: v10.16.3
• lint-staged: 9.4.3

[iiroj]

EDIT: Oops sorry, replied to the wrong issue.

[iiroj]

So if I understood correctly, you're wishing for a feature that when enabled, will skip committing if there are any modifications done by tasks? So you'd first run with something like --bail-if-modified, check, and then run again without the flag if everything seems good?

That should be totally doable, but I'd add it after v10 because it should be a lot simpler.

[jorgeorpinel]

Sorry for the delay! I was traveling. (Will be more responsive now.)

You understand correctly. But there would be no need to use the flag only the first time. The workflow would be:

1. git commit triggers hook that runs lint-staged tasks.
2. task (formatter, linter) modify staged files, and this causes lint-staged to error out without committing.
3. Developer can review the changes done by the tool by using git diff and works on them again or simply uses git add to accept these changes.
4. Repeat. (Usually, a 2nd time will succeed as the same task won't need to modify files again.)

[okonet]

I'm just wondering why do you want to review those changes? Is there any particular reason?

[jorgeorpinel]

Control and visibility. In order to be responsible for code changes I'm submitting, I want to know what they are. 😄
Also, I've definitely come across bugs or problems in formatters e.g. when Prettier and ESLint conflict with each other and overwrite each other's changes. Can't 100% trust automatic tools.

[jorgeorpinel]

Ping. Any ideas or conclusions about this, Iiro, Andrey, or anyone else? 🙂

[okonet]

My opinion is

1. tools that offer automatic fixes are well tested and produce good results.
2. you should not treat lint-staged as the only quality assurance tool. Add a CI step that runs lints etc since pre-commit hook can be skipped by the user.
3. Code review process should catch unnecessary changes.

What you're proposing is an interactive mode that shows you changes made by tools before they get added to index and I'm not sure I'd like lint-staged to be responsible for that. Reason is that it adds a significant complexity to the lib and pushed responsibility to maintainers. That being said, if you have an idea how to introduce such a feature without adding too much of complexity to lint-staged, I'm all ears.

Maybe @iiroj has an opinion on that too?

[iiroj]

In v10 this is easy, because we automatically do the git add . step. With the --bail flag we would just fail at that point instead of continuing. I can add this if you're fine with it, @okonet.

[okonet]

I'm fine with that if that solves the use case. I'd rather see an interactive mode but as I mentioned before, it's too complex to have IMO.

[iiroj]

I think for this to be useful, lint-staged should add all the modifications to the index, and then exit with code 1 without reverting to the original state and dropping the backup.

This way the user would see the tasks modifications in the staged files, but still have the option of reverting those by manually using the backup stash.

[jorgeorpinel]

What you're proposing is an interactive mode... if you have an idea how to introduce such a feature without adding too much of complexity...

I'm just proposing a --bail flag (the way it works in pretty-quick, basically).

for this to be useful, lint-staged should add all the modifications to the index, and then exit with code 1

Hmmm I think it should NOT stage any of the automatic changes, because the user already has their changes staged, so if there are automatic changes (which remain unstaged), you can then compare them easily with git diff after lint-staged exits with code 1 (preventing the commit). Not sure if I'm misunderstanding you though, @iiroj, because I'm not aware of this "backup stash" you're talking about.

Thanks a lot guys!

[jorgeorpinel]

From what I'm reading in the README, the backup stash doesn't change my position on this. Let the user manually stage the automatic changes by tasks, after lint-staged notices them (with --bail) and exits with 0, otherwise I'm not sure what the point is in having the flag, which would be equivalent to me just exiting from the commit message without saving (causing Git to abort the commit, but leaving everything staged).

[okonet]

Okay that actually make sense to me now. Let’s try to keep changes unstated and exit with a special code?

[jorgeorpinel]

Hi! Just a note: I just realized the option I may be trying to replicate from pretty-quick is --no-restage. Not sure, both options seem to have the same description to me... I use both in my current setup. (See also azz/pretty-quick/issues/96.) Thanks.