[CHORE]: Follow up with our custom pre-commit issues for open source #55

New issue

Open

opened 2026-04-16 01:15:06 +00:00 by imAsparky · 0 comments

imAsparky commented

2026-04-16 01:15:06 +00:00

Owner

What needs to be done?

Issue: Improve Pre-commit + Tox Setup Guide

Background

We have an internal guide (how-to-pre-commit-tox.md) documenting our pre-commit + tox integration. This issue captures improvements to the guide and contributor setup before the project goes open source.

CI tests have been intentionally removed due to prohibitive runtime (10–20x longer than local runs). As a result, the pre-push hook is now the primary automated safety net before code reaches the remote. This makes the guide and the contributor setup process more important to get right.

What Needs To Be Done

1. Document hook latency expectations

Tox has a startup cost even with warm environments — typically 3–5 seconds per hook on a fast machine, longer on slower ones. The guide should set this expectation explicitly so contributors aren't surprised.

Where: Add a note in Section 2 (.pre-commit-config.yaml) or a new "Performance" section.

2. Clarify the `language: system` assumption

The hooks use language: system, which requires tox to be available on PATH. This is a safe assumption for our project since deps are expected to be in place, but it should be stated explicitly so contributors know what's expected of their environment.

Where: Add a note under the hook that uses language: system, or in the Prerequisites section.

3. Explain the two-commit behaviour after auto-format

When tox -e format modifies files during a commit, pre-commit stages the changes but the commit still fails — by design. The contributor must review the changes and run git commit a second time. This surprises people the first time.

Where: Add a one-paragraph explanation in Section 4 (How it works), under the git commit flow.

4. Note that `repo: local` makes `pre-commit autoupdate` a non-issue

Contributors familiar with the standard pre-commit workflow may wonder why there are no pinned versions in .pre-commit-config.yaml and whether pre-commit autoupdate is needed. The guide should briefly confirm this is intentional — tool versions are managed via tox, not pre-commit.

Where: Add a short note in the Philosophy section or as a callout in Section 2.

5. Clarify that environment names must match `tox.ini`

The .pre-commit-config.yaml references tox -e format, tox -e lint, and tox -e py313. These names must match the actual environment names defined in tox.ini. Contributors using different naming conventions will hit confusing errors.

Where: Add a note in Section 2, near the YAML block.

6. Add a `dev-setup` make target (or equivalent)

The pre-commit and pre-push hooks must be installed separately:

pre-commit install
pre-commit install --hook-type pre-push

Contributors who only run the first command will have no pre-push hook, meaning tests won't run before pushing. Since CI no longer runs tests, this is a meaningful gap.

Add a single setup target — e.g. in a Makefile or justfile — that runs both commands, and reference it from the contributing guide and the how-to.

Suggested target:

.PHONY: dev-setup
dev-setup:
	pip install pre-commit tox
	pre-commit install
	pre-commit install --hook-type pre-push

Where: New section in the how-to ("Quick Setup"), plus a corresponding entry in CONTRIBUTING.md or equivalent.

7. Document the decision to remove CI tests

Since the pre-push hook is now the primary test gate, it's worth recording the rationale in the guide or a separate ADR (Architecture Decision Record) so future contributors understand the tradeoff and don't re-add CI tests without that context.

Key points to capture:

CI test runtime was 10–20x longer than local runs
Contributors are expected to have deps in place
The pre-push hook covers the gap for day-to-day development
What is not covered: matrix testing across Python versions or operating systems — document the supported/tested configuration explicitly

Where: A new "CI and Testing Philosophy" section in the how-to, or a separate docs/decisions/ ADR file.

Acceptance Criteria

Hook latency expectation documented
language: system assumption stated in prerequisites or inline
Two-commit auto-format behaviour explained in the how-to
repo: local / no autoupdate rationale noted
Environment name dependency on tox.ini called out
dev-setup target added and referenced from the contributing guide
CI test removal rationale documented (how-to section or ADR)

Scope

Tooling

Definition of Done

merge into main

Additional Context

No response

### What needs to be done? # Issue: Improve Pre-commit + Tox Setup Guide ## Background We have an internal guide (`how-to-pre-commit-tox.md`) documenting our pre-commit + tox integration. This issue captures improvements to the guide and contributor setup before the project goes open source. CI tests have been intentionally removed due to prohibitive runtime (10–20x longer than local runs). As a result, the `pre-push` hook is now the primary automated safety net before code reaches the remote. This makes the guide and the contributor setup process more important to get right. --- ## What Needs To Be Done ### 1. Document hook latency expectations Tox has a startup cost even with warm environments — typically 3–5 seconds per hook on a fast machine, longer on slower ones. The guide should set this expectation explicitly so contributors aren't surprised. **Where:** Add a note in Section 2 (`.pre-commit-config.yaml`) or a new "Performance" section. --- ### 2. Clarify the `language: system` assumption The hooks use `language: system`, which requires `tox` to be available on PATH. This is a safe assumption for our project since deps are expected to be in place, but it should be stated explicitly so contributors know what's expected of their environment. **Where:** Add a note under the hook that uses `language: system`, or in the Prerequisites section. --- ### 3. Explain the two-commit behaviour after auto-format When `tox -e format` modifies files during a commit, pre-commit stages the changes but the commit still fails — by design. The contributor must review the changes and run `git commit` a second time. This surprises people the first time. **Where:** Add a one-paragraph explanation in Section 4 (How it works), under the `git commit` flow. --- ### 4. Note that `repo: local` makes `pre-commit autoupdate` a non-issue Contributors familiar with the standard pre-commit workflow may wonder why there are no pinned versions in `.pre-commit-config.yaml` and whether `pre-commit autoupdate` is needed. The guide should briefly confirm this is intentional — tool versions are managed via tox, not pre-commit. **Where:** Add a short note in the Philosophy section or as a callout in Section 2. --- ### 5. Clarify that environment names must match `tox.ini` The `.pre-commit-config.yaml` references `tox -e format`, `tox -e lint`, and `tox -e py313`. These names must match the actual environment names defined in `tox.ini`. Contributors using different naming conventions will hit confusing errors. **Where:** Add a note in Section 2, near the YAML block. --- ### 6. Add a `dev-setup` make target (or equivalent) The pre-commit and pre-push hooks must be installed separately: ```bash pre-commit install pre-commit install --hook-type pre-push ``` Contributors who only run the first command will have no pre-push hook, meaning tests won't run before pushing. Since CI no longer runs tests, this is a meaningful gap. Add a single setup target — e.g. in a `Makefile` or `justfile` — that runs both commands, and reference it from the contributing guide and the how-to. **Suggested target:** ```makefile .PHONY: dev-setup dev-setup: pip install pre-commit tox pre-commit install pre-commit install --hook-type pre-push ``` **Where:** New section in the how-to ("Quick Setup"), plus a corresponding entry in `CONTRIBUTING.md` or equivalent. --- ### 7. Document the decision to remove CI tests Since the pre-push hook is now the primary test gate, it's worth recording the rationale in the guide or a separate ADR (Architecture Decision Record) so future contributors understand the tradeoff and don't re-add CI tests without that context. Key points to capture: - CI test runtime was 10–20x longer than local runs - Contributors are expected to have deps in place - The pre-push hook covers the gap for day-to-day development - What is **not** covered: matrix testing across Python versions or operating systems — document the supported/tested configuration explicitly **Where:** A new "CI and Testing Philosophy" section in the how-to, or a separate `docs/decisions/` ADR file. --- ## Acceptance Criteria - [ ] Hook latency expectation documented - [ ] `language: system` assumption stated in prerequisites or inline - [ ] Two-commit auto-format behaviour explained in the how-to - [ ] `repo: local` / no `autoupdate` rationale noted - [ ] Environment name dependency on `tox.ini` called out - [ ] `dev-setup` target added and referenced from the contributing guide - [ ] CI test removal rationale documented (how-to section or ADR) ### Scope Tooling ### Definition of Done merge into main ### Additional Context _No response_

imAsparky self-assigned this

2026-04-25 12:00:03 +00:00