Improvements to our commit-hook (lint-staged)

[arikfr]

• Switch to using prettier-eslint for linting and formatting files along with calling git add when done.
• Run Prettier on CSS/Less files as well.
• Make sure the messaging about what's going on is clear, as it might be confusing to new developers.
• Run flake8 when the developer using our Docker setup

[ranbena]

You mean git add cause you wanna run it with --fix?

[jezdez]

To echo my question in #3390 (comment), can we add a kill switch to all of these hooks to optionally disable it?

[ranbena]

Other than -n?

[jezdez]

@ranbena Yes.

[ranbena]

I think you can just remove the relevant files from .git/hooks.

[jezdez]

Would that have to happen on individual npm install runs?

[ranbena]

Indeed. Every husky install would bring em back.

[arikfr]

Indeed. Every husky install would bring em back.

That's not a solution :-)

Although I'm not sure if having a solution that permanently disables this makes sense. 🤔 It's either we think it's necessary or we don't.

[arikfr]

@ranbena btw, how about we run tests only on push and linting on commit? Linting should be fast on a few files and almost a no-op, while tests are a bit heavier.

[jezdez]

There is the HUSKY_SKIP_INSTALL env var that when set during installation prevents the hooks to auto-installed in the .git directory.

There is the node husky uninstall command that uninstalls the hooks.

Both things don't really prevent this from persisting between runs of npm install.

[ranbena]

not sure if having a solution that permanently disables this makes sense.

Since it's overridable anyhow, I don't see a problem with allowing devs to disable it comfortably. If there's a technical way I'd be happy to review such a PR.

we run tests only on push

It's a trade-off. On pre-push the full suite would have to be run which is a waste.

[jezdez]

Indeed. Every husky install would bring em back.

That's not a solution :-)

Although I'm not sure if having a solution that permanently disables this makes sense. 🤔 It's either we think it's necessary or we don't.

I couldn't find an explanation in #3390 why this commit hook is necessary to be honest. Could you elaborate what the advantages are?

[arikfr]

@ranbena,

Since it's overridable anyhow, I don't see a problem with allowing devs to disable it comfortably. If there's a technical way I'd be happy to review such a PR.

The problem is one of the message it sends: it's either we think it's important or we don't. If we don't, then we can just provide instructions to how to setup hooks for whoever interested instead.

@jezdez,

I couldn't find an explanation in #3390 why this commit hook is necessary to be honest.

1. To make sure we get consistent code in terms of formatting (helps with reviews later on).
2. Provides earlier feedback to the developer about lint errors. It's not uncommon to see lint errors ignored. If the feedback provided early in the process (or fixed automatically) it's easier to handle.

[jezdez]

FWIW, to reiterate what I said before: it would be great to have this hook not auto-installed and/or a way disable it easily. And no, remembering how to disable commit validation/verification on every commit isn't enough.

An example for how this breaks things for me, a screenshot of the Tower Git app that I use to commit things when I want to split changes up into individual commits or have to handle the rebases of our fork:

[ranbena]

An example for how this breaks things for me, a screenshot of the Tower Git app

Yeah Tower Git has issues with hooks. Have you tried mitigating this?

[jezdez]

An example for how this breaks things for me, a screenshot of the Tower Git app

Yeah Tower Git has issues with hooks. Have you tried mitigating this?

This isn't really an issue Tower has, it simply uses a PATH environment variable without the local node_modules directory. I don't think it's reasonable to having to maintain a custom, global plist file just to be able to commit to this repo.