Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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

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.
114 changes: 114 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# 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. 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
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. 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 <number>
```

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.

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.

### If you don't

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.

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 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

`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.
7 changes: 5 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,13 @@ You can reach us at <mailto:github@amplify.com>

## 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, 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.

Expand Down
Loading