Skip to content

chore: add lint pre-commit hook #6697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pkey wants to merge 2 commits into
base: main
Choose a base branch
from
+22 −0
Open

chore: add lint pre-commit hook #6697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3eb007
chore: add lint pre-commit hook to catch issues before CI
pkey Apr 1, 2026
198147a
Merge branch 'main' into feat/add-pre-commit-hook
pkey Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,23 @@ repos:
rev: v8.17.0
hooks:
- id: gitleaks
- repo: local
Copy link
Copy Markdown
Contributor

@PeterSchafer PeterSchafer Apr 1, 2026

Choose a reason for hiding this comment

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

Suggestion: Nice idea! did you intentionally not use npm run format?

Copy link
Copy Markdown
Contributor

@PeterSchafer PeterSchafer Apr 1, 2026

Choose a reason for hiding this comment

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

The only worry I have is performance/latency it adds.

Copy link
Copy Markdown
Contributor Author

@pkey pkey Apr 1, 2026

Choose a reason for hiding this comment

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

@PeterSchafer yeah I originally had npm run lint which I believe covers formatting and is also what we run in the code-analysis job. The issue with that is that it will run the linter/formatter on all the files every time.

This approach limits it only to what has changed before committing. Performance wise, we can't go any further I believe.

Copy link
Copy Markdown
Contributor Author

@pkey pkey Apr 1, 2026

Choose a reason for hiding this comment

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

Locally on my 32GB M1 the whole npm run lint runs in 7s - so that's the absolute worst case scenario.

Copy link
Copy Markdown
Contributor Author

@pkey pkey Apr 1, 2026

Choose a reason for hiding this comment

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

I guess the latency is not exactly linear (1 file still takes 2.5s) but still worth it IMO given that this can easily be 20+ minutes before you find out you have a linter error in the CI.

image

Copy link
Copy Markdown
Contributor

@j-luong j-luong Apr 2, 2026

Choose a reason for hiding this comment

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

I think running the same commands as in the code-analysis step should suffice. i.e. npm run lint & make lint.

I agree that the ~15seconds it takes to run the pre-commit linting is better than waiting for the CI job to fail

Copy link
Copy Markdown
Contributor

@PeterSchafer PeterSchafer Apr 2, 2026

Choose a reason for hiding this comment

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

okay let's run an experiment. when it gets too long, we can still change it!

Copy link
Copy Markdown
Contributor Author

@pkey pkey Apr 2, 2026

Choose a reason for hiding this comment

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

@j-luong so would you prefer the full npm run lint command or this version? This version I believe covers everything though if you would change what npm run lint does it would fall through the cracks.

hooks:
- id: lint-eslint
name: eslint
entry: npx eslint --quiet --cache
language: system
types_or: [javascript, ts]
pass_filenames: true
- id: lint-prettier
name: prettier
entry: npx prettier --check
language: system
types_or: [javascript, ts, json, yaml, markdown]
pass_filenames: true
- id: golangci-lint
name: golangci-lint
entry: bash -c 'cd cliv2 && CGO_ENABLED=1 make lint'
language: system
files: ^cliv2/
pass_filenames: false
2 changes: 2 additions & 0 deletions scripts/install-dev-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ set -exuo pipefail

# requires https://brew.sh/
brew bundle --file=$(dirname "$0")/Brewfile

pre-commit install
Loading