[TB] Add pre-commit hook to enforce prettier, linting

[aaronreed708]

Problem/Concern

Stemming from https://github.com/finos/a11y-theme-builder/pull/948#issuecomment-2200477209, it was suggested that we should have a pre-commit hook in Theme Builder to enforce Prettier formatting and linting so that we know that code being sent for code review won't have to spend much time on formatting issues.

Proposed Solution

Create a pre-commit hook so that committed code will have the correct formatting and linting.

[aaronreed708]

I created a pre-commit hook script for Git here: https://github.com/finos/a11y-theme-builder/pull/948#issuecomment-2221502034. However, such a script has to be installed by each developer OR we commit the hook into a folder in our repo and ask developers to update their Git config (as outlined here: https://stackoverflow.com/questions/427207/can-git-hook-scripts-be-managed-along-with-the-repository).

@kburk1997 suggested that we should use Husky, which can be included in package.json and installed when Theme Builder is built. So there would be no explicit action required of each developer. Of course, this will install code that will execute invisible to developers, which may not be welcome by all developers.

[budhathokipramin]

Hi! Please assign me this issue.

[aaronreed708]

From our discussion on Slack in the #a11y-theme-builder channel (starting at: https://finos-lf.slack.com/archives/C061D0HDHM2/p1721142690229599?thread_ts=1721108616.261539&cid=C061D0HDHM2)

pre-push not pre-commit - in my experience, personally, and from talking to team members it's too much. Devs use staging to split stuff and run fast iterations on POCs while in-flow, it blocks that, plus, changing format mid-work, affects pattern-recognition and causes cognitive drag, ie. the code I just staged looks different because of some lint thing, so now my head has to deal with that

that's sort of part my experience (highly pattern-recognition based) but also expressed by quite a few colleagues pre-push is less frequent and makes everything is nice and clean while it's still local and in the dev's mental cycle ... letting it through to CI though can give them long enough to lose the mental position they're in - start on the next task and switch context, forget stuff etc.

This makes sense to me. Letting the developer get done with fixing his issue or implementing his feature and THEN clean up the code. However, the one downside that I see is that I might change 20 files and just when I think I'm finally ready for a code review I now have to fix eslint warnings and errors in those files. But I would only have to do that once (instead of the half dozen to a dozen or so commits I did when changing those files). And if I am running the eslint stuff in my editor, then I should catch issues well before then anyhow.

So we should change this to be a pre-commit hook? Using Husky (or similar npm package-based approach)?

[aaronreed708]

Hi! Please assign me this issue.

Thank you @budhathokipramin for volunteering to address this issue! We will discuss this issue in our call tomorrow and make sure that everyone agrees with this approach and then you can start implementing. I will make sure to note in this issue what the final outcome was.

[aaronreed708]

Discussed in today's call. Everyone is fine with using pre-push hook rather than pre-commit. The suggestion was given that we should only fail push on ESLint error and not for warning. In that case we need to make sure that the things we care about are treated as errors, not warnings. The one example I can think of (warning that should be treated as an error) is the copyright headers.

[budhathokipramin]

Ok, I will work on this issue.

[budhathokipramin]

@aaronreed708 Have you Checked the above PR? Thank you.

[aaronreed708]

Update: PR has been reviewed and awaiting changes from @budhathokipramin.

[budhathokipramin]

@aaronreed708 I will inform you soon about the changes.Thank you.