CR follow-ups on #210: pin the charset claim, parse for the import, correct three docs - #215
Merged
Merged
Conversation
The docstring claimed the test pins agreement between two tables that could drift entry by entry. Deleting the local table is what ended that drift; the equality now holds by construction and cannot fail while extension_for is a bare delegation. What it really catches is a local branch or normalisation step reintroduced ahead of the call, which is worth keeping and worth naming accurately - a test whose stated purpose is unachievable reads as coverage the suite does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
extension_for's docstring asserted that every value co-core yields is lower-case alphanumeric, in the same commit that deleted _EXTENSION_SAFE - the local check which enforced exactly that. The claim became a fact about another repository's data with nothing here to catch it changing. Now asserted over the same inputs the delegation test uses, hoisted into _MEDIA_TYPES_CHECKED so the two cannot drift apart, and widened to the four table entries neither test covered (application/xhtml+xml, application/xml, text/xml, image/gif). Nothing depended on the claim for safety: RenderOccasion.values re-checks every occasion value against _SEGMENT_SAFE, so an unusable answer raises InvalidOccasionError rather than writing a malformed key. But it would raise at replication time, pointing at a table this repo does not own. The docstring now says that instead of asserting the property outright. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
assert "import mimetypes" not in source read straight past `from mimetypes import guess_extension` and `import mimetypes as mt` - the two spellings most likely to appear if someone reintroduces the fallback - and would trip on any comment containing the phrase, which this module's own block already runs close to. Substring matching over source is the technique CR 9-11 replaced with an AST scan for this same package, for this same reason. Reuses _imported_modules from the layering guard rather than writing a second scanner: it resolves aliases and relative forms, and its own planted-import tests keep it honest, so the detector is proven elsewhere instead of trusted here. A new planted test covers all three mimetypes spellings. _EXTENSIONS moves from a substring check to an AST scan of assigned names, keeping the hasattr check beside it - one sees a definition the other cannot (shadowed or conditional), and vice versa. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"helper. Deriving evaluates" sat alone mid-paragraph, breaking the column the rest of the file keeps and making the diff read as a truncation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t tree The destination.py entry credited rep_fields.py for the _slug companions but described the extension as simply "implied by the origin's media type", which now under-describes a cross-repo dependency of exactly the same kind. The tree is where someone looks to find out which module owns what. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Setting this worktree up, I replaced the .wheelhouse directory with a symlink to the main checkout's, to skip re-downloading 324 wheels. The directory is not untracked: .gitignore carries `.wheelhouse/*` plus `!.wheelhouse/.gitkeep` precisely so the find-links directory exists at checkout, because uv reads that config on every invocation and errors if it is missing (archiver#116). So `rm -rf` deleted a tracked file and `git add -A` committed the symlink in its place. In CI that symlink dangles - it names a path on this VM - and every job that touches the wheelhouse died at setup: lint, test and client-drift all failed in under 40s on `Failed to read --find-links directory`. Locally nothing showed, because locally the target exists. Restores .gitkeep and links the wheels individually inside the directory, which `.wheelhouse/*` ignores. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Review of
00d9dd9..c88bbd1(the #210 delegation). Six findings, five applied, one observation. No behaviour change — the onlysrc/edit is a docstring.c3b25edextension_forasserted a charset property of co-core's data that nothing pinned, in the commit that deleted the guard enforcing it1ab8fbfmimetypesguard matched source text, missing two import spellings272c616docs/BUS.mdparagraph left ragged by the pin correctionc30de3fdocs/ARCHITECTURE.mddid not name co-core as the extension table's ownerecc9105uv.lockgainedsdistentries the 0.13.2 pins lackedThe two that mattered
Finding 2 is the one with teeth.
extension_for's docstring said "every value it yields is lower-case alphanumeric" — a claim about another repository's data, written in the same commit that deleted_EXTENSION_SAFE, the local regex which had been enforcing exactly that. Nothing depended on it for safety (RenderOccasion.valuesre-checks every occasion value against_SEGMENT_SAFE, so an unusable answer raisesInvalidOccasionErrorrather than writing a malformed key) but the failure would have surfaced at replication time pointing at a table this repo does not own. Now asserted over the same inputs the delegation test uses, hoisted into one tuple so the two cannot drift, and widened to the four table entries neither test covered:application/xhtml+xml,application/xml,text/xml,image/gif.Finding 3:
assert "import mimetypes" not in sourceread straight pastfrom mimetypes import guess_extensionandimport mimetypes as mt— the two spellings most likely to appear if the fallback returns — and would trip on prose containing the phrase, which this module's own comment block already runs close to. Now reuses_imported_modulesfrom the layering guard, whose planted-import tests keep the scanner honest, plus a new planted test covering all three spellings. Substring-matching over source is precisely what CR 9–11 replaced with AST for this same package.Verification
ruff checkclean,ruff format --checkclean, 1772 passed / 2 skipped (the two hook tests that skip outside the main checkout) / 3 deselected. Two net-new tests.🤖 Generated with Claude Code