feat: set up husky pre-commit hook for linting

[JeffreytheCoder]

What this PR does

Currently, EC devs have to manually run linting check. Automatic linting check happens in CI/CD, which happens only after a PR is opened and would take minutes to run.

If an EC dev forgets to manually run linting check, the linting error would be found late in the development workflow. The dev has to push a new commit to fix the linting error, which happened many times in our previous PRs.

Pre-commit hook runs automatic linting check before an EC dev commits a piece of code. This allows EC devs to find the error early and avoid development workflow delays.

Video/Screenshots

[Spiral-Memory]

Hey @JeffreytheCoder, great work!

There was a thread in which Husky was discussed and avoided. I am not sure why, or maybe they were looking for the CI build first.

Another thread where Akshun discussed the same:

https://open.rocket.chat/channel/embeddedchat?msg=bobc8WgGMi4cqaTar

[Spiral-Memory]

Also, I personally wouldn't want this to happen automatically. Imagine I'm working on something and I want to stash or commit something temporarily, and I know there's something wrong with it. It will prevent me from committing, which isn't good in my opinion. I do understand that sometimes we forget to run lint checks, but it's fine; we'll know about it once the CI build finishes, and we can correct it then. However, while working on code, temporary commits, and stashing are things we do frequently. So it would be very restrictive in that sense.

I feel that might be the reason for maintainers as well, and @Akshun-01 stated the same in the thread by referring to a video.

[JeffreytheCoder]

Interesting...I think auto lint check should happen earlier, as explained in the PR description, while check in CI is the last barrier for enforcement.

@sidmohanty11 What do you think?

[JeffreytheCoder]

Also, I personally wouldn't want this to happen automatically. Imagine I'm working on something and I want to stash or commit something temporarily, and I know there's something wrong with it. It will prevent me from committing, which isn't good in my opinion. I do understand that sometimes we forget to run lint checks, but it's fine; we'll know about it once the CI build finishes, and we can correct it then. However, while working on code, temporary commits, and stashing are things we do frequently. So it would be very restrictive in that sense.

I feel that might be the reason for maintainers as well, and @Akshun-01 stated the same in the thread by referring to a video.

We can change this to pre-push hook, but it will take another commit to fix the errors. The hook is just an option for people who want to have auto check, and you can disable using --no-verify. Also we're going to add e2e tests in CI so it's gonna be slow.

Check out the comments in that video 🤣

[Spiral-Memory]

Ohh, I wasn't aware that we can disable it, if that's the case, then there won't be any problem imo.

Let's see what maintainers have to say to it and why they initially avoided it.

Edit: I think pre-push will be better. It's fine to add another commit; we usually do it after the CI build too. Also, if one doesn't want to make an extra commit, one can amend the existing commit using git commit --amend and then force push.

[Spiral-Memory]

Hey @JeffreytheCoder Can you please change it to pre-push hook, and then it can be merged !

[JeffreytheCoder]

Hey @JeffreytheCoder Can you please change it to pre-push hook, and then it can be merged !

Updated

[Spiral-Memory]

Hey @JeffreytheCoder ,

How do I get it to work? It's not working. I pushed a commit without any checks on my local system; it just got pushed to the remote branch.

Can you please make a video showing that it works when a commit is pushed to remote? I may be missing something

Edit : It started working, had to run yarn prepare for some reason !

[JeffreytheCoder]

@Spiral-Memory Just saw your comment. Glad it's working for you :)