Retire five duplicated implementations, three of them the stdlib's job - #40
Draft
imnasnainaec wants to merge 12 commits into
Draft
Retire five duplicated implementations, three of them the stdlib's job#40imnasnainaec wants to merge 12 commits into
imnasnainaec wants to merge 12 commits into
Conversation
lxml exposes no byte offsets, but the stdlib's expat binding does: CurrentByteIndex reports where the current event's markup begins. Taking region starts and ends from element events retires the tag, comment, CDATA, processing-instruction and nesting walk that found them by hand — the module drops from 189 lines to 137, and scanning sango.lift (4.8 MB) from 660 ms to 185 ms, since the byte loop it replaces ran in Python and expat runs in C. _tag_end survives, and does the one thing offsets alone cannot settle. An empty element's end event reports the offset just past the whole element, where every other element's reports the "<" of its end tag; the two cases are indistinguishable from the offset, so which one applies is read off the start tag's "/" instead. The same quote-aware scan supplies the root's open-tag end. Conservative refusals are unchanged, and now come from one place: expat rejects the malformed and truncated markup the walk used to detect case by case, and a DTD is refused as before, since entity expansion would make these offsets describe bytes that are not in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both replace a hand-rolled scan with the standard-library operation it spells out. _nearest_entry is called once per schema error and walked the entry table looking for the last entry starting at or before the error's line. The table is built in document order, so it is sorted and bisect_right finds that entry directly; a document failing validation on every entry no longer costs entries x errors. Filtering the lineless entries out once, before the loop, is also what gives the bisected list a total order to search. duplicate-form-lang asked langs.count(lang) per language of a multitext, rescanning the list once for every form in it. Counter answers the same question in one pass, and _writer already imports it for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_iter_multitexts, _iter_traits and _iter_grammatical_infos were the same recursion over dataclasses.fields with the isinstance target swapped, so they become one _iter_instances(obj, cls) and each caller names the class it wants. Why each of the three reaches as far as it does is now stated in one place rather than argued separately in each copy. The field label the multitext check reports was only ever produced by one of the three, and is now carried through the list branch as well: a match found inside a list field is labelled with that field rather than left unlabelled. The unified walk descends into a match instead of returning at it. That descent is what finds a Multitext inside a Multitext's own annotations, which the multitext copy already relied on; nothing in the model nests a Trait or a GrammaticalInfo inside another, so the other two callers see the same instances in the same order as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five places carried their own depth-first walk over sense.subsenses: two in the CLI, two on the model, one in validation. They become one method, which answers a question callers outside this package have as much as the ones inside it — Entry.senses holds only the top level, so anything asking about "the entry's senses" has to recurse. The CLI's leaf-sense pass is a filter over it now: with the walk in document order, the senses with no subsenses are exactly the rows export wants, in the order it wants them. media_refs() and the missing_media() list derived from it therefore report sibling senses in document order. The stack walk they used popped from the end, so sibling illustrations came out reversed within an entry — visible in check-media output, and in nothing that depended on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The class hand-wrote get, __contains__, keys, values and items on top of __getitem__ / __iter__ / __len__ — which is the set collections.abc.Mapping derives from those three. Inheriting it drops them, and makes isinstance(mt, Mapping) true for consumers that ask. keys(), values() and items() therefore return views rather than lists. A view is what a Mapping promises, and typing the class as one while returning lists would have been three suppressed Liskov violations; set operations on keys() work now, and a caller wanting a list can say so. __iter__ and __len__ read forms directly, since the views are built on them. __len__ counts languages rather than forms. It counted every form including a lang=None one, which keys() has always excluded, so len(mt) could exceed len(mt.keys()) on schema-invalid input; as a Mapping that would leave len(mt) != len(list(mt)). __bool__ still answers "is there anything to serialize", which residue and a lang-less form each defeat on their own, so it stays independent of len(). The two mutators stay as they are. MutableMapping is not inherited: clear and popitem have no clear meaning for a form list that can hold forms no key reaches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every module under src/ carries `from __future__ import annotations` except _extras, which needs nothing from it and so went unnoticed. Ruff's isort required-imports enforces what was being eyeballed, and adds the import there. The rule is scoped to the package: tests and scripts mostly do without it, and that is their own convention rather than a lapse in this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both belong in the object-model entry: what a multilingual field supports is most of what a caller does with this model, and that Entry.senses holds only the top level is the surprise all_senses() exists to answer. The media helpers' entry gains the two facts a caller has to know to read their output: every subsense is covered, and references come out in document order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A language repeated across forms was yielded once per form, so the inherited views walked it twice and resolved both to the first form's text: values() reported that text twice and never the second form's, and len() counted 2 where dict() held 1 key. A repeated language is exactly the schema-invalid input validation reports as duplicate-form-lang, so it is real FLEx and WeSay output rather than a hypothetical. __iter__ now yields each language once — the one __getitem__ answers with — and __len__ counts those, so len(mt) == len(dict(mt)) whatever the forms hold. Nothing is hidden: forms still holds every form, which is where duplicate-form-lang reads from and where a lang-less form was already the docstring's example of content no mapping can represent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Whether the package's `from __future__ import annotations` convention is worth enforcing with a linter is a preference, settled by argument rather than by any test, and it shares nothing with the duplicated implementations the rest of this branch retires — no other commit here touches pyproject.toml or _extras. It is proposed on its own in #41 instead, where accepting or rejecting it costs nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
changed the title
Both were true, and both were true somewhere else. _nearest_entry bisects the entry table, which needs it ordered by line. That held because entry_lines is built from the parsed document's root children in order — a fact about _group_children_by_tag leaving the root alone, two functions away from the bisect that depends on it. Sorting at the call site states the requirement where it applies. The sort is keyed on the line rather than comparing whole tuples, which would raise as soon as two entries sharing a line differ in having an id. _iter_instances descends into a match rather than stopping there, which is harmless only while the model nests no Trait inside a Trait and no GrammaticalInfo inside a GrammaticalInfo. That was established by reading the dataclasses; the new test establishes it from the annotations, so gaining such a field fails the suite instead of quietly widening what the walk yields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inheriting collections.abc.Mapping changes three things callers can see: keys(), values() and items() return views rather than lists, len() counts languages rather than forms, and a language spelled on two forms becomes one key. Accepting that is a judgement about how much of a 0.x API is worth breaking for a correct protocol, which nothing here shares a file with — the rest of this branch deletes duplicated code without changing what anything returns. It is proposed on its own in #42, so it can be taken or refused without holding up six changes that are only refactors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
changed the title
The guard's stated reason was that "byte scanning assumes an ASCII-compatible encoding", describing a scan that no longer exists. The reason it still holds is different and less obvious: expat parses UTF-16 quite happily and reports byte offsets into UTF-16 bytes, tag names and region boundaries and all, so nothing downstream can tell those regions apart from usable ones. Reused verbatim they leave an unchanged document identical to its source and an edited one mixing both encodings, which will not parse at all. The ranges-side guard, which carried no reason, gets one pointing at that. 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.
Resolves #39 — five duplicated implementations retired, one commit each.
_scan.py→ expat'sCurrentByteIndex, which lxml has no equivalentfor. 227 lines to 137, and 3.6x faster: 660 ms to 185 ms on
sango.lift._nearest_entry→bisect, so a document failing schema validation onevery entry stops costing entries x errors.
duplicate-form-lang→Counter, notlist.countper language._iter_instances(obj, cls).Entry.all_senses(), public becauseEntry.sensesholds only the top level.Worth your attention
The scanner rewrite, since it underpins byte identity. Both scanners agree
exactly, region for region, across 26 crafted documents (BOM,
>inside anattribute value, CDATA-embedded
</entry>, DOCTYPE, six truncations) and all57 corpus files. The subtlety to look hardest at: an empty element's end event
reports the offset past the element, where every other element's reports the
<of its end tag, and the offsets alone cannot tell those apart — the starttag's
/decides it. Only 6 of the 26 cases distinguish a wrong rule there,and none of the 57 corpus files does.
media_refs()ordering changed. Sibling senses come out in document ordernow; the old stack walk reversed them within an entry. No test pinned it, and
check-mediaoutput is the visible difference.python scripts/check.pygreen: 575 passed, 97%+ coverage. Why eight otherhand-rolled things are deliberately left alone is in the follow-up comment
on #39.
🤖 Generated with Claude Code
Devin review: https://app.devin.ai/review/sillsdev/python-sil-lift/pull/40
This change is