refactor!: introduce the adapter contract - #8
Merged
Merged
Conversation
The interface an api object had to satisfy was never written down: it was whatever deepl-rb happened to expose. AdapterContract is that interface as a test module, so any adapter is validated by including it.
Extract DeepLDiff::Adapters into its own file (lib/deepl_diff/adapters.rb) instead of declaring the module inline in lib/deepl_diff.rb behind a Style/OneClassPerFile disable. The disable masked a real fix: giving the module its own file removes the second top-level module declaration that tripped the cop, with no behavior change. Replace test_translate_preserves_order's refute_equal check, which only proved two outputs differ (a reordering adapter passed it), with a test that compares a batched translate call against per-element individual calls at the same positions. This also catches ordering bugs that a naive reversed-input/reversed-output check would miss, since a full array reversal is its own inverse and cancels out under that comparison.
Unwrapping #text and downcasing the detected language move out of Request and into the adapter, where they belong. deepl-rb stays out of the gemspec: the client is an argument and ::DeepL resolves at call time.
MAX_CHUNK_SIZE and COUNT_LIMIT were DeepL's numbers hard-coded into the core. Every provider has its own, so the adapter declares them and the chunker requires them.
from and to were keys to be dug out of a positional hash with Hash#delete, which is what made 2.1.0 copy the caller's hash. Keywords collect a fresh hash per call, so both the copy and the bug it worked around are gone.
The prior test only re-verified 2.1.0's dup-the-hash fix, which cannot distinguish itself from this task's change; add a test asserting the positional options hash is rejected with ArgumentError, and rename the old test to say what it actually covers. Also stop erasing from/to from DeepLDiff.translate's signature: fully anonymous keyword forwarding satisfied rubocop but made the gem's only public entry point self-undocumenting. Name from: and to: explicitly and forward the rest anonymously.
api.translate(sample, nil, to) returned a different type than the same method with a non-nil from -- one method, two return types, told apart by an argument value. Detection is its own method now, and is optional: an adapter without it makes from: required, with a message that says so.
Generalising exposed what one provider hid. The key knew nothing about the provider, so switching providers against the same Redis returned the old one's translations, and formality: :less shared a key with the default. Language codes are downcased while we are here, since this release invalidates every existing entry anyway.
Hash#inspect changed how it renders symbol-keyed hashes in Ruby 3.4, and an option value without its own #inspect embeds a memory address. Either silently changes the cache key across a Ruby upgrade or between processes, making the application pay for every translation again. Canonicalise primitives, arrays and hashes explicitly instead, and raise on anything else so an unstable key is impossible rather than surprising. Also pins the positional contract between cached_and_missing's keys and the store's response, since a database-backed store answering an IN query will not return rows in request order.
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.
Six tasks of the generalisation work. The rename to
translation_difffollows in a separate PR.The gem has contained no reference to DeepL since 2.0.0 — the API object is supplied by the application and duck typed on one method. But the duck type was never written down (it was whatever
deepl-rbhappened to expose), and DeepL's request limits were hard-coded into the chunker. This PR fixes both, and one correctness bug that generalising exposed.The adapter contract
AdapterContractis the interface as an executable test module. Any adapter is validated by including it and definingadapter. Two adapters ship:Adapters::DeepL(current behaviour) andAdapters::Null(tests).deepl-rbdoes not become a dependency. The adapter takes its client as an argument and::DeepLresolves at call time — the same technique that already keepsredis-namespaceandratelimitout of the gemspec. Runtime dependencies remainoxandpunkt-segmenter.Detection is its own method, and optional
Before this,
api.translate(sample, nil, to)returned an object of a different type than the same method with a non-nilfrom— one method, two return types, told apart by an argument's value. Nowdetect(text)is separate, and an adapter that does not implement it makesfrom:required with a clear message instead of dying withNoMethodError.Limits belong to the provider
MAX_CHUNK_SIZE = 1700andCOUNT_LIMIT = 300were DeepL's numbers living in the core. They are now declared by the adapter and required byChunker. With one provider a default looks meaningful; with two it silently lies about the second — and this file already shipped a bug of exactly that shape in 2.1.0.Options are keyword arguments
translate(values, from:, to:, **options). This deletes theoptions.dupadded in 2.1.0 along with the class of bug it worked around: keyword arguments collect a fresh hash per call, so the caller's hash can no longer be consumed.Cache keys, and a bug generalising exposed
The key gains the provider, a digest of the provider options, and normalised language codes:
Without this, switching providers against the same Redis silently returns the first provider's translations, and
formality: :lessshares a key with the default.from: "EN"andfrom: :enalso stopped producing two entries for identical work.The options digest is built on a canonical encoding, not
#inspect. Ruby 3.4 changed how symbol-keyed hashes render, so an#inspect-based digest gives different keys on different supported Rubies — upgrading Ruby would have silently re-paid for every translation. Verified that 3.2.4 and 4.0.5 now produce byte-identical keys. Any option value that cannot be encoded stably raises rather than producing an unstable key.Important
Every cache key changes. Nothing cached by earlier versions is reused.
Test plan
bundle exec rake test— 74 runs, 126 assertions, 0 failures (was 46 before this work).bundle exec rubocop— no offences, with no cop disabled anywhere to achieve it.