lonboard icon indicating copy to clipboard operation
lonboard copied to clipboard

Document and unify pre-commit hooks for Python and JavaScript

Open vgeorge opened this issue 2 months ago • 5 comments

Problem

The project's pre-commit setup is not documented, causing confusion and CI failures (e.g., in PR #906). Checks are not enforced locally for all contributors.

Suggested Solution

  1. Unify Hooks: Extend the existing .pre-commit-config.yaml to include JavaScript checks (ESLint, Prettier) alongside the Python checks (Ruff).
  2. Document Setup: Create a CONTRIBUTING.md or update DEVELOP.md to instruct contributors on how to install and use the hooks with uv run pre-commit install.

vgeorge avatar Oct 10 '25 11:10 vgeorge

Indeed we should add a section in DEVELOP.md suggesting to use uv run pre-commit install to install pre-commit hooks locally.

  1. Unify Hooks: Extend the existing .pre-commit-config.yaml to include JavaScript checks (ESLint, Prettier) alongside the Python checks (Ruff).

Pre-commit is a fully "static" lint approach, meaning that it has access to your code, but no dependencies. Pre-commit runs in isolated environments with only your code and the thing being run in that step.

This is why on the Python side, code format and linting is applied but not type checking, because that needs access to installed dependencies to work correctly.

We could probably add TS code formatting to pre-commit, but I assume we'd run into the same issue for type checking in TS that we do in Python.

Pre-commit is meant as an easy way to do a limited number of static checks, not all checks that might be run on CI

kylebarron avatar Oct 10 '25 13:10 kylebarron

Might be worth switching to lefthook, which we use in veda-ui to lint and type check. It’s auto-installed on yarn install and pretty fast. Looks like it can be set up to run Python checks too.

vgeorge avatar Oct 13 '25 15:10 vgeorge

As I understand it, this problem is universal regardless of what tool you use to run pre-commit checks.

Assuming that lefthook uses isolated environments, it won't have Python dependencies available when type checking, which means that type checking will be wrong. (The same issue exists for Typescript, because if you don't have dependencies installed when you run tsc, it won't catch any bugs when you interface with dependencies).

And if lefthook didn't use an isolated environment, then it would give different results whether you run it locally or in CI, which would be equally bad.

We should add information to DEVELOP.md but I don't see anything else actionable here.

kylebarron avatar Oct 13 '25 16:10 kylebarron

It’s possible to guarantee exact dependency versions for both Python and JavaScript with yarn install --frozen-lockfile and uv sync --frozen, ensuring lefthook runs in the same environment locally and in CI.

vgeorge avatar Oct 13 '25 16:10 vgeorge

I see; so lefthook doesn't use isolated environments like pre-commit does.

I don't have a strong opinion here. I'd be fine with just documenting the existing state. But also open to changing the pre-commit provider if you want to implement that. I just want pre-commit checks to be fast (<1-2 seconds) so I don't want to e.g. run all of tsc on pre-commit (but maybe there are newer, faster type checking implementations?).

(We ran really slow checks like tsc on deck.gl/kepler.gl and it meant that a commit took like 40 seconds to run, so I ended up just always force-pushing my commits)

kylebarron avatar Oct 13 '25 17:10 kylebarron