Repository navigation
Comment and docstring conventions, and a pass applying them - #9
Merged
Merged
Conversation
The comments added with the output-rules document argued their case four times over. Why both format sets are published was spelled out at OUTPUT_MODELS, again on ImageOutputRules.formats, again on PdfOutputRules.formats and again in the document builder; it is now made once, where the sets are defined, and pointed at from the one field whose purpose is not obvious from its name. The handler's comment on which error answers for detail, code and rule ran ten lines, most of it a history of the bug and a hypothetical about moving a check into a field validator. Both belong to the commit that made the change. What survives is the rule itself and why it is not errors[0]. Also drops a reference to the test file that reads the published lists — a pointer that rots the moment the test moves — and trims two test docstrings and the mirror's that restated what the assertions below them already show. No behaviour change: 717 passed, 2 skipped; ty and ruff clean.
…rings Twenty-four :param: lines, and most restated the parameter name and its annotation: "renderer: The Typst renderer instance", ":param count: The number of keys the caller sent". A reader who needs those is reading the signature anyway, and a docstring that opens with four of them teaches them to skip the paragraph above, which is where this codebase keeps its reasons. Removed where they restate, kept where they add what the annotation cannot — that key reaches the caller's error context, that source_bytes is already UTF-8, that _resource_limit_args returns empty when the limit is unset. Every :raises: is kept: exceptions are not in the signature, so those lines are the only statement of what escapes. AGENTS.md now carries the rules rather than leaving them to be rediscovered per review. They are written against this codebase's habit of explaining at length: the licence to do so is exactly what makes empty prose expensive.
Thirteen docstrings of eight lines or more, read one at a time. Ten keep every line: they carry what the code cannot say — why a failed prlimit wrapper answers 422 to everyone and pages nobody, why --data must not be a separate argv token, why measuring base64 without decoding may call a whitespace payload too large rather than malformed. Length was never the defect. Three carried a sentence the docstring had already made: _resource_limit_args stated the empty case in its summary and again in :return:, _write_inline_files explained the escape check in a paragraph and again in :param:, and validate_inline_file_key narrated that each check below says what it rules out. The scripts/ module docstrings name tests/test_examples.py, which the new rule appeared to forbid. They are right and the rule was too broad: naming a test as the consequence of ignoring an instruction is not the same as naming one as evidence. The rule now says which is which.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comments and docstrings only. No behaviour change.
The convention, written down
AGENTS.mdgains a Comments and docstrings section. The rules are written against thiscodebase's habit of explaining at length, because that licence is what makes empty prose expensive:
a docstring opening with four lines that restate the signature teaches a reader to skim the
paragraph above it, which is where the reasons live.
Six rules: write what the code cannot say; say it once, where the decision lives; no
:param:or:return:that restates the signature, but keep every:raises:; cross-file pointers rot; bughistories belong in the commit message; a docstring's first line says what the thing is for.
Applied to the comments added in #6
The same argument — why both format sets are published, so a mirror never reads "not pdf, therefore
image" — was made at
OUTPUT_MODELS, again onImageOutputRules.formats, again onPdfOutputRules.formats, and again in the document builder. Now made once, where the sets aredefined.
The comment above
reportedinapp/main.pyran ten lines: the rule, the bug behind it, thePydantic validator ordering that makes the bad case unreachable today, and what would happen if a
check moved into a field validator on
files. Four lines remain — which error answers fordetail,codeandrule, and why it is not simplyerrors[0]. Also gone: a comment namingtests/test_output_rules.pyfromoutput_rules.py, and two test docstrings that restated theassertions below them.
Applied to the docstrings
Twenty-four
:param:lines existed; most restated the name and the annotation —:param renderer: The Typst renderer instance,:param count: The number of keys the caller sent. Removed wherethey restate, kept where they add what the annotation cannot: that
keyreaches the caller's errorcontext, that
source_bytesis already UTF-8, that_resource_limit_argsreturns empty when thelimit is unset, that a
dsnofNonedisables Sentry. Every:raises:is kept — exceptions arenot in the signature, so those lines are the only statement of what escapes.
This pass covered
app/render/renderer.py,app/api/routes.pyandapp/render/templates.py, whichheld 20 of the 24.
app/core/sentry.pyandapp/core/errors.pykeep theirs, all of which carryinformation.
Verification
uv run pytest(717 passed, 2 skipped, integration included),uv run ty check app scripts,uv run ruff check .andmake docs-buildall pass.