Retire five duplicated implementations, three of them the stdlib's job - #40
Open
imnasnainaec wants to merge 13 commits into
Open
Retire five duplicated implementations, three of them the stdlib's job#40imnasnainaec wants to merge 13 commits into
imnasnainaec wants to merge 13 commits into
Conversation
imnasnainaec
changed the title
imnasnainaec
changed the title
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>
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>
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>
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>
The ranges guide now runs load, read, edit, save without a detour through resolution. The three candidate sources move below the standalone example as a bulleted "Companion discovery", where the resolve_ranges=False opt-out already sat stranded at the foot of the section, and gain the case/normalization matching rule — which the guide had never stated, leaving validate.md's ambiguous-ranges-file row as the only prose that mentioned it, and that only from the failure side. The changelog drops that same matching rule, which says how discovery resolves rather than what the release offers, and no longer calls the vendored ranges schema the first of its kind: the entry says what ships, the guide says how it behaves. The comment on the sort feeding _nearest_entry's bisect moves onto the sort, off the filter that precedes it, and stops noting that list.sort is stable — documented Python, and the tie that settles is already described by _nearest_entry itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bulk-edit worked example built its own recursive generator to reach glosses under subsenses, which all_senses() now does. The note that explained the recursion becomes a note about the method, and the entry.senses pitfall it warned about moves to a sub-bullet under it. The model tour introduced senses as entry.senses[0] and said nothing about nesting, leaving the partial accessor as the one a reader meets first and all_senses() reachable only through the generated API reference. Its sense block now shows both, with the comments saying which covers what, and a line on which one to reach for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
imnasnainaec
force-pushed
the
stdlib-and-internal-duplicates
branch
from
September 3, 2026 14:01
0f6b5e1 to
2f133f7
Compare
No corpus document carries a ">" inside an attribute value, so replacing _tag_end's quote tracking with a plain search for the delimiter passed the whole suite. Ignoring the "/" that marks an empty element passed every test but the Hypothesis round-trips, which report it as a byte-identity failure far from its cause; the existing self-closing test covers the root, which appends no region at all. Three cases now fail deterministically instead. An empty child needs both rules right at once, because its region ends at the measured tag end rather than at the end event, so that case covers either mistake alone. The non-empty child in the attribute-value case is asserted as well: its end comes from the end event, so a tag end stopping inside an attribute value still leaves its region correct. 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.
_scan.py→ expat'sCurrentByteIndex, which lxml has no equivalentfor. 189 lines to 140, and 3.3x faster: 340 ms to 103 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. The corpus
byte-identity tests cover all 57 documents (53
.liftplus companions), andtests/test_scan.pydrives the scanner directly for what the corpus cannotreach.
offset past the element, where every other element's reports the
<ofits end tag. The offsets alone cannot tell those apart, so the start tag's
/is what decides it.>inside anattribute value either, so both rest entirely on the three cases added to
tests/test_scan.py. An empty child needs both rules right at once, sinceits region ends at the measured tag end rather than at the end event.
Sense walk order changed. Sibling senses come out in document order now;
the old stack walk reversed them within an entry. Two surfaces show it:
media_refs()/missing_media(), socheck-mediaoutput reorders.missing-sense-idand sense-relationdangling-reffindingsreorder.
iter_problems()never sorts, so callers see generator order; thewalk order shifts for five corpus entries carrying sense relations.
Out-of-scope
Why eight other hand-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