refactor!: replace the pipeline with a design of our own - #20
Merged
Conversation
…ped twice encode_entities escaped every &, so an entity outside the three decode resolved came back escaped again -- > rendered as &gt;, and an & inside a notranslate element was respelled the caller never asked for. Decode named and numeric references, encode only & and <, and move both to Segment, which is what knows whether it was translated: untranslated renders the bytes it was cut from, translated renders equivalent markup.
A translated sentence holding ©, — or ’ rendered the entity spelled out, because CGI.unescapeHTML knows only the specials and the numeric forms, so encoding escaped the & it left behind. Ox knows every name and is already a dependency. Resolve one name at a time, alone in an element of its own -- whole prose raises on a lone & and returns garbage for an out-of-range reference -- and keep numeric references, and the invalid-UTF-8 guard, on the CGI path.
Batch.pack fills batches against a provider's declared max_batch_size and max_request_size (escaped length), and Batch#apply attaches a reply to the segments that produced it by the batch's own index rather than by position matched afterwards. Not wired into the pipeline yet.
SentenceCache derives its cache key from a digest of the sentence's raw, pre-decode text plus any per-call provider options -- the format is pinned against the pipeline-baseline recorded keys so existing users don't miss their entire cache on upgrade. fill and store both hand back fresh arrays rather than draining the collections they are given.
The per-call options were folded into the sentence digest, so a caller passing formality: or a glossary id computed a key no cache could ever hold. The format has five fields when options are present and four when they are not, which is why the recorded baseline -- captured without options -- could not catch this. Canonicalise the options by sorting on the key's string form: @options.sort raised ArgumentError on a hash mixing Symbol and String keys. Render values with #inspect and raise a named SentenceCache::Error when one renders as an object address, rather than emitting a key that changes every process and reports nothing. Hash Segment#body instead of a local copy of Segment's whitespace-boundary regex. The two agreed, which is the reason to close it: this branch has already been bitten by two hand-synchronised definitions of padding.
The separator could not be observed from a single-option key, so the last
pass picked one. Running the pipeline being replaced against a recording
store shows it is ",": "&" gives 92c55e66 where the old pipeline asks for
c1ee2461, and every caller passing two options misses their whole cache.
Pin one-option, two-option and out-of-order-key hashes as literals. The
two-option case is the first that has anything to put a separator between,
and {b: 2, a: 1} is the only one whose literal order differs from its
sorted order, so it is what proves the sort rather than a lucky ordering.
A Hash or Array option was rendered with #inspect, giving Ruby literal syntax where the pipeline being replaced recurses. DeepL's splitting_tags and ignore_tags are list-valued and options splat straight through translate, so those callers were missing their whole cache. Recovered the shape by capturing nine option hashes from the old pipeline through a recording store and brute-forcing 5,625 delimiter, separator and pair-joiner combinations against all nine digests at once. One reproduces them all: containers carry no delimiters, a Hash canonicalises the way the options hash does, an Array its elements in order, both joined with ",". It is lossy -- ["x", ["y", "z"]] and ["x", "y", "z"] collide -- and that is the interface, not a defect to fix here. Replace the address-sniffing denylist with an allowlist of the types the format can render. A Struct and a Set have a tidy #inspect and no address, so they were being given keys the old pipeline refuses to build, which is the silent wrong key the error exists to prevent.
TranslationDiff.translate and Context#translate now build a Translator. Linearizer, Spacing, Chunker, Tokenizer, Cache and Request are deleted, along with their tests: they derived from google_translate_diff, which carries no licence at all, and this gem ships under MIT. The replacement -- Document, Leaves, Passage, Fragment, Segment, Markup, Batch, SentenceCache and Translator -- was written from tests and a spec without reading them. Cache keys are unmoved except where the CHANGELOG says otherwise, pinned by test against values recorded from the pipeline being replaced. The README's attribution to GoogleTranslateDiff goes with this commit and no other: it was true while the code was derived, and stops being true here.
…sertion Batch::Filler#ensure_sendable! raised the bare TranslationDiff::Error, the only pipeline failure without its own class, so a caller could not catch "sentence too long for this provider" apart from an unrelated registry miss. Add Batch::Error and document it alongside Translator::Error and SentenceCache::Error. pipeline_corpus_test.rb dropped its document: assertion for the five EXPECTED_TO_CHANGE inputs, leaving only echoed: checked against itself via two paths. Restore document: as its own literal, independent of echoed:, so a regression in Translator or Passage#render is caught by its own assertion rather than being invisible behind one value checked twice.
…directory The corpus test read its baseline from a path outside the repository, under the author's home directory. It passed on the machine that wrote the file and failed everywhere else: CI reported 27 errors, all `Errno::ENOENT`, on both supported Ruby versions. That path came from a standing rule that design documents stay out of git, applied to something that is not a design document. The baseline is test data -- the pipeline's recorded output and the cache keys it asked for -- and a test whose fixture exists on one laptop is not a test. It carries nothing that needed keeping out: the sentences are invented for the corpus and the keys are digests of them, with no credential and no customer text anywhere in the file.
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.
Why this exists
Six files in this gem derived from
google_translate_diff:request.rb,linearizer.rb,spacing.rb,chunker.rb,tokenizer.rbandcache.rb. That project carries no licence at all — noLICENSEfile,nothing in its gemspec, and none reported by GitHub. All rights reserved.
This gem ships under MIT, which nobody here is in a position to grant for
code that is not theirs.
This branch deletes all six, and their tests, and replaces them with a
pipeline of our own.
How the replacement was written
From the tests and a written spec, without reading the files being
replaced. The clean-room constraint bound every commit on this branch that
wrote the new pipeline; only this final commit — the switch-over, the
deletion and the docs — was written with the old files open, and only to
check what referenced them. Nothing was copied out.
The first commit on the branch (
0af12af) pinned the old pipeline'sbehaviour before any of it changed: 32 corpus inputs, each a case some
earlier bug or review turned up, translated through the
:nullprovider andrecorded to a baseline file.
test/translation_diff/pipeline_corpus_test.rbjudges the new pipeline against that baseline, input by input.
A reviewer compared the two designs and found the replacement independent
work.
What replaces what
LinearizerDocument(walks a caller's structure without flattening it) andLeavesTokenizerPassage,FragmentandMarkup(markup and prose, without rebuilding either)SpacingSegment(a sentence keeps the whitespace it was found in)ChunkerBatch(packs sentences into provider-sized requests)CacheSentenceCacheRequestTranslatorTranslationDiff.translateandContext#translatekeep their signatures.docs/how-it-works.mddescribes the new pipeline.What changes for a caller
Cache keys are otherwise unmoved.
SentenceCache's key format is pinnedby test against values recorded from the pipeline being replaced
(
test_keys_match_the_ones_the_previous_pipeline_producedand friends intest/translation_diff/sentence_cache_test.rb), including the optionsdigest's canonicalisation, which is reproduced collision-for-collision rather
than fixed — changing it would move every key.
Five corpus inputs change, and they were declared in
PipelineCorpus::EXPECTED_TO_CHANGEbefore the rewrite, on purpose, so thelist could not be assembled after seeing what broke:
Salt & pepper.was sent to the provider as the six characters&— the provider translated the entity's spelling as words, and wepaid for it. Now sent as
Salt & pepper.and re-encoded on the way out.Hard space here.likewise.if a < b then stop. Fine.—oxread the<as an unclosed tag, soonly
if awas ever translated. A<that opens no tag is now escapedbefore parsing and restored after.
5 < 6 and 7 > 6. True.— sent as three fragments, now sent as twosentences.
a <b then stop. Fine.— a recorded limit, not a fix:<bcannot betold apart from a tag without a lexer of our own.
Two further key changes, both in the CHANGELOG:
keys as the bare sentence. The pipeline uses one Unicode-aware definition of
padding everywhere; the old key was built with ASCII
strip, which leavesU+00A0in place.<still reaches a provider undecoded, a known limit of working aroundoxrather than replacing it.Documents containing any of these will miss the cache once and be
re-translated. That is the point: what was cached for them was translated from
the wrong text.
Also breaking, and in the CHANGELOG:
Request::Error→Translator::Error,Cache::Error→SentenceCache::Error,Chunker::Errorgone (the condition it named nowraises
TranslationDiff::ErrorfromBatch). No aliases: this gem hasnever been published under the name
translation_diffwith those constantsin it. All three stay
TranslationDiff::Error, so a rescue of the baseclass is unaffected.
to:missing ornilraisesArgumentErrornaming the keyword. Itpreviously compared equal to a
nilsource, short-circuited as "samelanguage", and handed your values back untranslated in silence.
cacheinstrumentation event fires once per call, not once per chunk —the cache is now consulted for every sentence in one
read_multibeforeanything is batched.
hitsandmissessum to the same totals, so acounter dashboard is unaffected; an event-rate counter or a per-chunk
histogram will notice.
The README line
is removed in this commit and no other. It was true while the code was
derived. It stops being true when the last derived file is deleted, which is
this commit.
Verification
changes the path every other test runs through. 502 runs, 1176 assertions,
0 failures, 0 errors, 0 skips.
bundle exec rubocop— 83 files, no offenses.google_translate_diffor the six deleted constantsremains in
lib,test,docsorREADME.md, outside the CHANGELOG's ownhistory and the rename note in
docs/errors.md.One behaviour test edited, and why
pipeline_corpus_test.rb'sCHANGEDtable carried adocument:columnrecording what the old path still produced for the five changed inputs, so
that half of each case held the line until the switch. With the old path
gone, the column is dead: the assertion now compares against
echoed:, whichTask 5 wrote for exactly this moment. Only
entity nbspactually differedbetween the two (
renders as the character it means — the samedocument to a browser, not the same bytes). No other behaviour test was
touched.