search

my-covid-story / www

We are a group of concerned citizens who could no longer stand by as Ontario is led into a humanitarian crisis. We believe the power of storytelling is an effective means to drive government action.
https://www.mycovidstory.ca
Creative Commons Zero v1.0 Universal
9 stars 5 forks source link

Do not format or add on pre-commit, just fail and let the dev do it #164

Closed davesteinberg closed 3 years ago

davesteinberg commented 3 years ago

The husky changes in PR #111 really threw me for a loop. It's very common for me to make a commit without staging everything, mostly when I'm shuffling bits of work around among different in-progress commits. Or just amending a previous commit to change the message while leaving some new changes unstaged. Having git add . in the pre-commit hook is really incompatible with how I work.

I suppose the change was made because if it doesn't make sense to have eslint fix your errors and prettier reformat your code and then not commit those changes.

This PR undoes that. Instead of fixing and reformatting, it just fails if there's a problem. I've renamed the existing lint script to lint-fix and added a new lint that doesn't fix, which is what's now being run in the pre-commit hook.

The idea is that if the hook fails, the commit is blocked, and the developer can run lint-fix, verify what it did, stage the changes, and retry the commit. Is this workflow okay with everyone, or are folks really attached to automatic fixing and staging?

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/my-covid-story/www/8SCBV2r5xbGdWArH13WyFAK2UADH
✅ Preview: https://www-git-dsprecommit-my-covid-story.vercel.app

JamiesonRoberts commented 3 years ago

@davesteinberg Yeah I agree with this. I get the original idea behind it, but doing a git add . should be an action by the user, not an automated thing.

davesteinberg commented 3 years ago

Hmm, I agree with not adding automatically but just failing lint and requiring an extra command doesn't make sense to me. Why not meet in the middle, auto fix but then not automatically add?

Because that will just commit the unfixed files, which I don't think we want, either.

davesteinberg commented 3 years ago

It occurs to me that I didn't mention @LuckeeDev on this. Apologies, I should have. What do you think of this change?

It also occurs to me that if anyone prefers the auto fix and auto add behaviour, they could create an alias to do it:

alias commit="npm lint-fix && git add . && git commit"
davesteinberg commented 3 years ago

Ooo, seems I somehow broke the fonts with this. I'm investigating on PR #166.

davesteinberg commented 3 years ago

Okay, it seems I didn't actually break the fonts, it was just some transient Vercel weirdness. I'm going to merge this while things are working again.