From efe14a87ddb2064b3d1c50b1c1938813085e6a6c Mon Sep 17 00:00:00 2001 From: Kamil Kozik Date: Mon, 24 Aug 2026 20:03:14 +0200 Subject: [PATCH 1/3] docs: add a contributing guide covering the pre-PR review workflow The README pointed contributors at "open a PR" with no mention of how to check their work first. This parser is bidirectional, so a change that looks right in isolation can break round-tripping somewhere else in the pipeline; the guide front-loads that. Documents the intended sequence: get the suite and pre-commit green locally, open a draft PR, verify it with /review-pr, then mark it ready. Draft first because the skill reviews a PR number rather than a working tree, and because it signals no human attention is wanted yet. Also records two things that reliably confuse people: no-commit-to-branch failing by design during --all-files on main, and fork PRs not running CI until a maintainer approves the workflow, which is why the local run carries the weight. Co-Authored-By: Claude Opus 5 (1M context) --- CONTRIBUTING.md | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 5 ++- 2 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..cd303d22 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,104 @@ +# Contributing + +Thanks for contributing to `python-hcl2`. For any sizable change, please open an issue first so we can agree on the approach before you spend time on it. + +The workflow below exists because this parser is bidirectional: almost every change has a counterpart somewhere else in the pipeline, and a change that looks correct in isolation can quietly break round-tripping. Running the checks locally and reviewing the PR before asking a human to read it catches most of that. + +## The short version + +1. Make your change, with tests. +1. Get the **full test suite** and **pre-commit** passing locally. +1. Open a **draft** PR. +1. Run `/review-pr ` against it and act on the findings. +1. Mark it ready for review. + +## 1. Set up + +```bash +python -m pip install --upgrade -r test-requirements.txt -e . +pre-commit install +``` + +## 2. Make the change + +Read [`CLAUDE.md`](CLAUDE.md) first. It documents the pipeline, the module map, and a set of hard rules that reviewers will check against — most importantly: + +- Always go through the LarkElement IR; never convert a Lark tree straight to a dict or back. +- Every serialization path needs a matching deserialization path. Parse → serialize → deserialize → serialize must produce identical output. +- One grammar rule maps to exactly one `LarkRule` class. +- Adding a language construct means touching `transformer.py`, `deserializer.py`, `formatter.py` **and** `reconstructor.py`, not just the grammar. + +Tests use `unittest.TestCase` — not pytest. Unit tests live in `test/unit/`, full-pipeline tests with golden files in `test/integration/`. + +## 3. Get it green locally + +Both of these must pass before you open a PR. + +**Tests:** + +```bash +python -m unittest discover -s test -p "test_*.py" -v +``` + +Run the whole suite, not just the tests you added. The integration suites (`test_round_trip.py`, `test_specialized.py`) are where round-trip regressions surface, and they are easy to break from a change that looks local. + +**Pre-commit:** + +```bash +git add -A +pre-commit run +``` + +With no arguments `pre-commit run` checks the staged files, which is what the commit hook will check. Stage first — a command like `--files $(git diff --name-only origin/main)` silently skips files you have added but not staged, so a brand-new module goes unchecked. + +Prefer that over `--all-files`. Two things to know about `--all-files`: + +- `no-commit-to-branch` fails whenever you run it while `main` is checked out. That hook exists to stop commits to `main`, so failing there is intended and not something to fix. +- It may surface pre-existing problems in files you never touched, which makes it hard to see whether *your* change is clean. + +If a hook fails and the fix isn't obvious, `/fix-precommit` will diagnose and fix it. + +To reproduce CI exactly — it runs the suite across Python 3.8 through 3.13 — use `tox`. + +## 4. Open a draft PR + +Open it as a **draft**. Two reasons: it signals you are not asking for human attention yet, and it gives `/review-pr` something to review, since the skill works from a PR number rather than a working tree. + +```bash +gh pr create --draft --fill +``` + +In the description, explain **why** the change is needed, not just what it does — the diff already shows the what. Link the issue it fixes (`Fixes #123`) and include a short test plan. + +## 5. Verify with `/review-pr` + +``` +/review-pr +``` + +This is a [Claude Code](https://claude.com/claude-code) skill defined in `.claude/skills/review-pr/`. It runs autonomously and then reports; it does not change your branch or post anything to GitHub without asking. + +What it does: + +| Phase | What it checks | +|---|---| +| Issue validation | Reproduces the linked issue's exact snippet and confirms your change actually fixes it | +| CLAUDE.md compliance | The hard rules above, plus the full checklist when a language construct is added | +| Code review | Per-area review of grammar, transformer, rule classes, reconstructor, deserializer and test coverage | +| Tests | Full suite, plus edge cases derived from what the review found | + +It finishes with findings graded Critical / Warning / Info, and asks how you want to proceed. Add `--skip-tests` to skip the test phase if you have just run it yourself. + +Treat the output as a first reviewer, not a verdict. It is good at the mechanical checks — a missing deserializer path, a test that passes without the fix applied, an edge case the change does not cover — and those are exactly the things that otherwise get caught late. It can also be wrong, so push back where you disagree. + +Fix what it finds, push, and re-run it if the changes were substantial. + +## 6. Mark ready for review + +Once the suite is green, pre-commit is clean and you have addressed the review findings, mark the PR ready. Note that CI does not run automatically on pull requests from forks until a maintainer approves the workflow, so your local run may be the only signal for a while — which is why step 3 matters. + +## Notes on conflicts + +`CHANGELOG.md` conflicts often, because every change appends to the same list. When you resolve it, keep both entries and put the one already on `main` first, so your diff stays a pure append. + +For anything else, prefer merging `main` into your branch over rebasing. It avoids a force-push and keeps your commits intact. diff --git a/README.md b/README.md index ce255029..f4d402c0 100644 --- a/README.md +++ b/README.md @@ -146,10 +146,11 @@ You can reach us at ## Contributing -We welcome pull requests! For your pull request to be accepted smoothly, we suggest that you: +We welcome pull requests! See [CONTRIBUTING.md](CONTRIBUTING.md) for the full workflow. In short: - For any sizable change, first open a GitHub issue to discuss your idea. -- Create a pull request. Explain why you want to make the change and what it's for. +- Get the test suite and pre-commit passing locally. +- Open a draft pull request, verify it with `/review-pr`, then mark it ready. We'll try to answer any PR's promptly. From 6d7ea3ac31140c797c68bfd666ac7832a4354af0 Mon Sep 17 00:00:00 2001 From: Kamil Kozik Date: Mon, 24 Aug 2026 20:16:02 +0200 Subject: [PATCH 2/3] docs: point CLAUDE.md at the contributing guide Two additions. Under Pre-commit Checks, the staged-files invocation and the no-commit-to-branch caveat, both of which cost time to rediscover. Under Keeping Docs Current, CONTRIBUTING.md joins the list of docs that have to move when the workflow does -- a stale command in the contributor entry point is worse than a stale line in CLAUDE.md, since a contributor has no way to know it is wrong. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 7ce75b76..2193d7d5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -199,6 +199,12 @@ Hooks are defined in `.pre-commit-config.yaml` (includes ruff, mypy, and others) - Ensure **mypy** passes. - End files with a newline; strip trailing whitespace (except under `test/integration/(hcl2_reconstructed|specialized)/`). +Run the hooks against staged files (`git add -A && pre-commit run`) rather than `--files $(git diff ...)`, which skips untracked files. `no-commit-to-branch` fails by design during `--all-files` while `main` is checked out. + +`CONTRIBUTING.md` documents the end-to-end contributor workflow this feeds into: local suite + pre-commit green, then a draft PR, then `/review-pr` before requesting human review. + ## Keeping Docs Current Update this file when architecture, modules, API surface, or testing conventions change. Also update `README.md` and the docs in `docs/` (`01_getting_started.md`, `02_querying.md`, `03_advanced_api.md`, `04_hq.md`, `05_hq_examples.md`) when changes affect the public API, CLI flags, or option fields. + +Update `CONTRIBUTING.md` when the contributor workflow changes — the test or pre-commit commands, the skills it references, or what CI runs. It is the entry point contributors read, so a stale command there costs more than a stale line here. From 418d60ed357d8a711db0502a9f299addef235b8b Mon Sep 17 00:00:00 2001 From: Kamil Kozik Date: Tue, 25 Aug 2026 11:51:27 +0200 Subject: [PATCH 3/3] docs: make the self-review step tool-neutral Two problems with the first draft. A bare `/review-pr` means nothing to someone who has never used Claude Code, and the README mentioned it with no indication of what it even was. Worse, it sat as step 4 of a five-step workflow, which read as gating contribution on a specific paid tool. This repo takes PRs from outside contributors; that is not a bar we want. The requirement is now the checklist itself, written so it can be worked by hand. The skill is presented as an automated shortcut for people who happen to have it, with an explicit note that nothing here needs Claude Code and no PR is held up for lacking it. The checklist is drawn from what has actually slipped through on this repo, led by "does your test fail without your source change" -- that one alone has caught a test asserting an unreachable value and another that never exercised the code path it named. Also notes that maintainers may run the skill during review, so a contributor can see up front what it will look for. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- CONTRIBUTING.md | 40 +++++++++++++++++++++++++--------------- README.md | 4 +++- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2193d7d5..6889bc1a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -201,7 +201,7 @@ Hooks are defined in `.pre-commit-config.yaml` (includes ruff, mypy, and others) Run the hooks against staged files (`git add -A && pre-commit run`) rather than `--files $(git diff ...)`, which skips untracked files. `no-commit-to-branch` fails by design during `--all-files` while `main` is checked out. -`CONTRIBUTING.md` documents the end-to-end contributor workflow this feeds into: local suite + pre-commit green, then a draft PR, then `/review-pr` before requesting human review. +`CONTRIBUTING.md` documents the end-to-end contributor workflow this feeds into: local suite + pre-commit green, then a draft PR, then a self-review checklist before requesting human review. Run `/review-pr` to work that checklist — for contributors it is optional, since it requires Claude Code, but here it is the expected path. ## Keeping Docs Current diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cd303d22..0e05af2e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,9 +9,11 @@ The workflow below exists because this parser is bidirectional: almost every cha 1. Make your change, with tests. 1. Get the **full test suite** and **pre-commit** passing locally. 1. Open a **draft** PR. -1. Run `/review-pr ` against it and act on the findings. +1. Self-review it against [the checklist](#5-self-review-before-asking-for-review) — by hand, or automatically if you use Claude Code. 1. Mark it ready for review. +No particular tooling is required. Step 4 has an automated shortcut for Claude Code users, but the checklist is the actual requirement and doing it by hand is perfectly fine. + ## 1. Set up ```bash @@ -70,32 +72,40 @@ gh pr create --draft --fill In the description, explain **why** the change is needed, not just what it does — the diff already shows the what. Link the issue it fixes (`Fixes #123`) and include a short test plan. -## 5. Verify with `/review-pr` +## 5. Self-review before asking for review + +Go through this before marking the PR ready. Every item is something that has actually slipped through on this repo. + +- **Does the linked issue's exact snippet now behave correctly?** Not a paraphrase of it — copy the code block from the issue and run it. +- **Does your test fail without your source change?** Stash or revert just the source edit, run the new test, confirm it fails, restore. A test that passes either way is not testing your fix. This is the highest-value check on the list. +- **Round-trip holds?** Parse → serialize → deserialize → serialize must produce identical output. If you added golden files, `json_serialized/` and `json_reserialized/` should be byte-identical for your suite. +- **Both directions updated?** A new serialization path needs its deserialization counterpart. A language construct needs `transformer.py`, `deserializer.py`, `formatter.py` and `reconstructor.py`. +- **Full suite green**, not just the tests you touched. +- **Edge cases**: empty bodies, nested constructs of the same type, interaction with interpolation, and the construct in a container (tuple element, object value, function argument). + +If the change touches the grammar, also check that no existing terminal can now match in a context it previously could not. + +### If you use Claude Code + +The [`/review-pr`](https://claude.com/claude-code) skill in `.claude/skills/review-pr/` automates the checklist: ``` /review-pr ``` -This is a [Claude Code](https://claude.com/claude-code) skill defined in `.claude/skills/review-pr/`. It runs autonomously and then reports; it does not change your branch or post anything to GitHub without asking. - -What it does: +It fetches the PR and its linked issue, reproduces the issue, checks the `CLAUDE.md` rules, reviews each changed area, runs the suite plus edge cases it derives from what it finds, and reports findings graded Critical / Warning / Info. It asks before changing anything or posting to GitHub. `--skip-tests` skips the test phase if you have just run it. -| Phase | What it checks | -|---|---| -| Issue validation | Reproduces the linked issue's exact snippet and confirms your change actually fixes it | -| CLAUDE.md compliance | The hard rules above, plus the full checklist when a language construct is added | -| Code review | Per-area review of grammar, transformer, rule classes, reconstructor, deserializer and test coverage | -| Tests | Full suite, plus edge cases derived from what the review found | +Treat it as a first reviewer, not a verdict — it is good at the mechanical checks and it can also be wrong, so push back where you disagree. -It finishes with findings graded Critical / Warning / Info, and asks how you want to proceed. Add `--skip-tests` to skip the test phase if you have just run it yourself. +### If you don't -Treat the output as a first reviewer, not a verdict. It is good at the mechanical checks — a missing deserializer path, a test that passes without the fix applied, an edge case the change does not cover — and those are exactly the things that otherwise get caught late. It can also be wrong, so push back where you disagree. +Work the checklist by hand; that is the whole requirement. Nothing in this project needs Claude Code, and a PR is never held up for lacking it. -Fix what it finds, push, and re-run it if the changes were substantial. +For reference, maintainers may run `/review-pr` on your PR during review. The checklist above is what it looks for, so working through it yourself means fewer round trips either way. ## 6. Mark ready for review -Once the suite is green, pre-commit is clean and you have addressed the review findings, mark the PR ready. Note that CI does not run automatically on pull requests from forks until a maintainer approves the workflow, so your local run may be the only signal for a while — which is why step 3 matters. +Once the suite is green, pre-commit is clean and you have worked the checklist, mark the PR ready. Note that CI does not run automatically on pull requests from forks until a maintainer approves the workflow, so your local run may be the only signal for a while — which is why step 3 matters. ## Notes on conflicts diff --git a/README.md b/README.md index f4d402c0..b9c45ec3 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,9 @@ We welcome pull requests! See [CONTRIBUTING.md](CONTRIBUTING.md) for the full wo - For any sizable change, first open a GitHub issue to discuss your idea. - Get the test suite and pre-commit passing locally. -- Open a draft pull request, verify it with `/review-pr`, then mark it ready. +- Open a draft pull request, self-review it against the checklist in the guide, then mark it ready. + +No particular tooling is required. If you happen to use [Claude Code](https://claude.com/claude-code), the repo ships a `/review-pr` skill that automates that self-review, but it is a convenience and not a requirement. We'll try to answer any PR's promptly.