docs(contributing): correct four small claims - #43
Conversation
The Python suite was described as covering only what no other implementation shares, over a four-item list. Its own module docstring gives five, adding the transcript skip, and the reason is wrong for part of the file besides: the matcher-level tests are shared, and the Rust carries the same vectors as unit tests, because a matcher is where a hand-written port diverges in a way no document catches. The version floor was quoted with double quotes where the manifest uses single ones, so a literal search for the quoted line found nothing. The notebook clause said no number is written into its prose. No *measurement* is, which is the property worth claiming and the one the cells enforce. Several counts of what a cell does are spelled out, and those move with the cell's constants, so the absolute version promised something the notebook does not deliver. And the nbconvert timeout was explained as a 30-second default raised. There is no per-cell default: nbconvert hands execution to nbclient, whose `timeout` trait defaults to none, measured against the versions this tree locks. The flag is a ceiling where there was none, which is a reason to keep it and a different reason than the one given. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR |
PR Summary by QodoCorrect contributing guide claims about tests and benchmarks
AI Description
High-Level Assessment
Files changed (1)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
=======================================
Coverage 87.40% 87.40%
=======================================
Files 3 3
Lines 691 691
=======================================
Hits 604 604
Misses 87 87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The section gave the command and never the occasion, and the gap is not theoretical in either direction. `ruff-check --fix` reaches the notebook through `types_or: [python, pyi, jupyter]`, so it can rewrite a code cell on commit with every stored output left alone -- which is what 50fc805 did, changing a source line and no execution count. And an output has been edited by hand rather than by running the notebook. Both leave the page describing code that is not above it, and a maintainer doing either has followed this document exactly. So it now names the three occasions: a code cell changing, including a change made for you at commit time; release time, because the outputs state the release list and the file counts as well as the timings; and never by hand, because the outputs are the page and only a run should write them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
Changes are documentation-only and the updates are consistent with the referenced repository sources, with only a minor wording nit noted.
Pull request overview
This PR updates CONTRIBUTING.md to correct four previously inaccurate or over-absolute documentation claims, aligning contributor guidance with what the repository actually does (tests, version floor declaration, benchmark notebook guarantees, and nbconvert timeout behavior).
Changes:
- Clarifies what
tests/test_unwrap.pycovers vs. what the corpus specifies, including the transcript-skip and matcher-level parity pins. - Corrects the quoted
requires-pythondeclaration to matchpyproject.toml. - Refines benchmark notebook statements to distinguish measured figures from prose counts, and updates the nbconvert timeout explanation; adds guidance to re-execute the notebook when lint fixes touch code cells.
File summaries
| File | Description |
|---|---|
| CONTRIBUTING.md | Corrects and tightens several contributor-facing claims about tests, version floor, and benchmark notebook execution guarantees. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Michael I Chen <michael.chen@aicadium.ai>
What changes, and why
Four small claims in
CONTRIBUTING.md, each recomputed.:33— whattests/test_unwrap.pycovers. "covers only what a corpus cannot describe, because no other implementation shares it: argument handling, file discovery, encoding failures, exit codes." Its own module docstring gives five items, adding the transcript skip. And the stated reason is wrong for part of the file: the matcher-level tests are shared —src/cli.rsandsrc/ignore.rscarry the same vectors as Rust unit tests, because a matcher is where a hand-written port diverges in a way no document would catch. The sentence now separates the two groups and says why the second exists.:43— the version floor. Quoted asrequires-python = ">=3.10"; the manifest isrequires-python = '>=3.10'(pyproject.toml:21). A literal search for the quoted line found nothing.:63— the notebook's figures. "Every figure in it is computed by the cell above it, so no number is written into the prose." No measurement is, which is the property worth claiming and the one the cells enforce. Counts of what a cell does are spelled out in the markdown — "times three programs that do almost nothing", "six of the eight measurements would fall within the first quarter", "times two runs" — and those track the cell's own constants. The absolute version promised something the notebook does not deliver, which matters because this sentence is what a reader relies on when deciding whether a prose number can be trusted.:73— the nbconvert timeout. "The default timeout is 30 seconds per cell, and several cells need longer." There is no per-cell default:nbconvert hands execution to nbclient, and the 30-second default belonged to nbconvert 5.x before that. So
--ExecutePreprocessor.timeout=1800is a ceiling where there was none, not a short limit raised — still worth passing, for a different reason than the one given, and a contributor was otherwise told to expect a hung cell to abort in 30 seconds when it would run forever.Corpus
The corpus is the specification, and both implementations answer to it. Tick what applies.
corpus/pins the new behavior. The case was written first and failed first.Checks
make checkpasses, ormake testdoes and this touches no Rust.make tidy, plusunwrap-markdown-prose-py --fail-on-change CONTRIBUTING.md. One Markdown file.