Skip to content

Conversation

khaledosman
Copy link
Contributor

@khaledosman khaledosman commented Mar 10, 2025

What's changing

Closes #951

Setup pre-commit hooks for linting & formatting changed frontend files before creating a commit using husky & lint-staged

How to test it

Steps to test the changes:

  1. you might need to run npm install from the frontend directory to install the new dependencies (lint-staged & husky)
  2. run make local-up
  3. make changes to backend and/or frontend files
  4. try to create a commit git commit -am "test commit"
  5. linting & formatting should run for both backend and frontend code

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@khaledosman khaledosman force-pushed the frontend-pre-commit branch from f125f69 to 5b2f883 Compare March 10, 2025 16:16
Makefile Outdated
Comment on lines 131 to 133
git config --unset-all core.hooksPath
uv run pre-commit install
pushd lumigator/frontend && npm run prepare && popd
Copy link
Contributor Author

@khaledosman khaledosman Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when running npm install it will now run the prepare script in

"prepare": "cd ../../ && husky install lumigator/frontend/.husky"

husky sets the core.hooksPath to frontend/.husky during its installation which breaks whatever checks uv pre-commit does when running make local-up (core.hooksPath defines the folder/path where github looks at to see where the hooks are defined)

I was getting this error when running make local-up

uv run pre-commit install
[ERROR] Cowardly refusing to install hooks with core.hooksPath set.
hint: git config --unset-all core.hooksPath
make: *** [local-up] Error 1

so this is a workaround to unset core.hooksPath before running uv run pre-commit install to make things work then run the npm prepare script to install husky (which sets git config core.hooksPath to frontend/.husky) and make the frontend pre-commit hook work alongside the backend one, where we run the uv run pre-commit run from frontend/.husky/pre-commit (so technically I think we can even get rid of running uv run pre-commit install in the make file since its already part of the pre-commit hook in .husky)

This feels hacky, but I'm not sure if there's a better way to do it. cc @macaab26, essentially all the frontend needs to do is run npx lint-staged from the frontend folder, so to avoid all this setting/unsetting hooksPath stuff, we could technically get rid of husky and rely on the normal .git/hooks/pre-commit however this file is automatically generated/overwritten by ruff so that wouldn't work either, perhaps we could create a custom pre-commit hook file and manually append to the generated pre-commit hooks or something along these lines which would also kinda hacky 🤷, or we could just rely on husky to run both backend & frontend linting which is what it currently does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed uv run pre-commit install in 4ce0d50 now that .husky/pre-commit is responsible for running linting for both backend and frontend where it would run uv run pre-commit run and npx lint-staged

@khaledosman khaledosman marked this pull request as ready for review March 10, 2025 16:46
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@khaledosman khaledosman requested a review from agpituk March 10, 2025 17:06
@khaledosman khaledosman added local-dev local development do-not-merge PRs that should NOT be merged while this label is present labels Mar 12, 2025
@khaledosman khaledosman changed the title chore: setup frontend pre commit hook [onhold] chore: setup frontend pre commit hook Mar 13, 2025
@peteski22 peteski22 marked this pull request as draft March 26, 2025 09:16
@macaab26 macaab26 removed their request for review July 18, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge PRs that should NOT be merged while this label is present frontend local-dev local development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setup pre-commit hooks for the frontend

1 participant