feat!: own the transport, give providers a base class, ship six of them - #17
Merged
Conversation
TranslationDiff::Provider replaces the duck-typed provider contract: subclasses declare configuration options/requirements and capabilities, translate a Translation::Request into a Translation::Response, and get a checked, all-or-nothing option registration and a guarded cache_key for free. Providers.register now rejects anything that isn't a Provider subclass, and Null is ported onto the new base class. DeepL and Google still predate Provider (Tasks 4/5 port them), so their unconditional registration at require time is rescued in translation_diff.rb rather than taking the whole library down; they're simply absent from the registry until then. Migrating Null also means every provider built through the registry now speaks the new contract, which request.rb (owned by a later task) doesn't consume yet -- three integration test files that routed through :null needed small local doubles in place of the now-incompatible provider to keep exercising their own behaviour without touching request.rb.
Providers.register's "not a Provider subclass" check raised the generic TranslationDiff::Error, which is also what ProviderOptionOwners raises for an option-name collision. The rescue in translation_diff.rb around the still-unported DeepL/Google requires caught both, so a genuine option collision on deepl_api_key or google_api_key would have been swallowed and misreported as the expected transitional state. InvalidProviderError narrows the rescue to exactly the case it's meant to cover; a collision now still takes the require chain down.
Add HTTPProvider: one Faraday connection every REST provider inherits, with HTTP-status-to-error mapping and no logging middleware, ever, so no line this gem writes can carry source text, translated text, or a credential. Rewrite the shared provider contract onto Translation:: Request/Response and provider.class.capabilities, and add the stubbed- provider and HTTP-provider-contract test helpers every REST provider's test will include. Pin json to < 3 in the Gemfile: json 3.0 dropped the positional `opts` argument Faraday::Response::Json still passes to JSON.parse, which broke every JSON response Faraday parses.
json 3.0 dropped the positional opts argument Faraday::Response::Json still passes to JSON.parse, and json 3 is now the default gem on Ruby 4.x -- so any application on a modern Ruby would have hit an ArgumentError on the first response this gem parsed. A Gemfile pin only protected our own suite, not the users who would ship with it. Stop installing faraday's response-JSON middleware and decode the body ourselves in #post, the same reasoning that took this gem off the vendor SDKs. #post now returns a small Decoded value (status, headers, already-parsed body) so #raise_for_status! and every subclass's #parse_translate_response keep the same shape they had. A non-JSON body -- an HTML error page from a proxy, for instance -- passes through untouched rather than raising.
The provider now speaks DeepL's REST API on this library's own Faraday transport. Dropping deepl-rb removes a dependency, but the reason is narrower than that: its defaults were not ours, and neither was its logging. It writes the whole request at DEBUG -- the Authorization header, auth key and all, followed by the payload, which is the text being translated -- so a customer's content reached the application log the moment anyone turned DEBUG on to diagnose something. Nothing we configure on our own connection does that. Two vendor facts are now stated where they can be read. The free host is selected from the `:fx` key suffix, which deepl-rb used to do for us. And `max_batch_size` is 50, DeepL's documented limit; the old code said 300, which the per-request size limit usually capped first -- a list of short values would have reached it and been rejected. `tag_handling: html` with `tag_handling_version: v2` is preserved exactly. It is what makes `class="notranslate"` work, and it failed silently for the whole life of the DeepL provider before it was added.
Ports the Google provider onto HTTPProvider, the same way DeepL was ported in the previous commit, and drops google-cloud-translate-v2 along with its transitive dependencies (googleauth, signet, os, google-protobuf, grpc). Also removes the transitional require-bridge in lib/translation_diff.rb now that both built-in HTTP providers are ported, and adds :google back to the built-in providers assertion.
Faraday's test adapter reuses one Env for both the request and the response, overwriting its body once the stub block returns -- so StubbedProvider was recording the reply, not the request, for every assertion made after #translate returned. DeepL and Google's tests already worked around this locally with `env.dup`; fix it here so Task 6 and beyond can use the shared helper as-is. Also let `body:` be a callable handed the request's env, so a provider's own `provider` test helper can echo back a response sized to match whatever texts a particular test sent, the way DeepL's and Google's local helpers already did.
The comment and test name still referenced "the transitional bridges in lib/translation_diff.rb", which are gone. What the test guards is unchanged: InvalidProviderError is specific to a provider of the wrong shape, so a caller rescuing that cannot also swallow an unrelated option-name collision, which stays a generic Error.
Azure AI Translator v3, the third HTTP provider on the shared transport. It differs from DeepL and Google in one structural way: the language pair and api-version/textType travel in the query string while the texts travel in the body, so #translate is overridden to build the URL per request. Billed characters come from the X-metered-usage response header. Limits (1000 strings, 50,000 characters per request and per string) are Azure's documented ones, the largest of the three providers here.
No ModernMT trial key was available in this environment, so the notranslate capability could not be probed against the live API and stays false with an "unverified, safe default" comment rather than a verified observation. RuboCop's Metrics cops rejected the brief's parse_translate_response verbatim (AbcSize, CyclomaticComplexity, MethodLength); extracted results_from/usage_for helpers to bring it under the limits without changing behaviour. Also renamed the "documented 128" test method to avoid Naming/VariableNumber.
Docker was available, so the notranslate capability was probed for real rather than left an unverified guess: against libretranslate/libretranslate --load-only en,ru, <span class="notranslate">Bold Mountain</span> is a good place. came back with the span tag intact but "Bold Mountain" translated to "Смелая гора" anyway. LibreTranslate's HTML format preserves markup; it does not honour the notranslate marker, so the constant is a documented false rather than a safe-default false.
Both files carried their own copy of the request-recording Faraday stub, written before StubbedProvider's body: callable existed. Now that the shared helper can echo request-sized responses, both migrate onto it, leaving the local copy in Azure's test as the only style and StubbedProvider as the one place the plumbing lives. Assertions are unchanged; only how each stub is built moved.
Wires the pipeline to the Provider/Capabilities/Translation::Request-Response contract every provider already speaks: Request#chunks reads Capabilities instead of two provider methods nothing defines any more, #detect_language checks capabilities.detects_language? instead of a respond_to? check that was true for every provider, and #call_api sends a Translation::Request and returns a Translation::Response, dropping the now-dead manual count check. Without this, TranslationDiff.translate raised NoMethodError for every provider on this branch. Also fixes a real aliasing bug the required chunking test surfaced: a provider that hands back the same array it was given (rather than a fresh one, as Null does) had that array silently drained to empty by Cache#store's destructive #shift, visible to anything else still holding the reference. call_api now dups its return value. Migrates the three test files still carrying pre-Provider doubles (instrumentation_test.rb, context_test.rb, request_test.rb) onto the real contract, preferring the actual :null provider where a double added nothing.
Every multi-line comment block in lib/ and test/ is reduced to at most one line, keeping the load-bearing fact (a decision, a vendor quirk, a measured observation) and dropping restated prose and usage examples.
Dedup savings from the magic comment are traded for plain mutable string literals; explicit .freeze keeps the constants that need to stay frozen. Ruby 3.4 chills string literals without the comment, so it becomes the supported floor.
A provider returning a well-formed response that carries nil for one input passed Response.build -- it checked only the count. The nil was written into the cache under a real key and the caller got NoMethodError: undefined method 'strip' for nil out of Spacing.restore, naming neither the provider nor the position. Azure documents exactly this shape: 200 for a batch where one element carries `error` instead of `translations`. Five of six providers can produce it since the branch replaced SDK objects with raw JSON lookups. Response.build now names the class and the position of the first offender, never the value, which is the customer's text or the provider's error object.
#store shifted its `updates` argument empty. Task 10 dup'd at the one call site, which fixes that caller and nothing else: #store is a public method on a public class taking an outside array, so the next call site anyone adds reintroduces the bug in the same silent form -- the caller's array comes back empty with no error. It now indexes instead of shifting. The dup at the call site stays: a provider's array is the provider's, and Request should not hand it to a collaborator either. Pins the exact cache keys so this change, and the option-declaration change that follows, are shown not to move one.
The design promised that DEEPL_AUTH_KEY, TRANSLATE_KEY/GOOGLE_CLOUD_KEY and TRANSLATE_PROJECT would be read by us now that the SDKs that read them are gone. Nothing implemented it and nothing could: configuration_options was a bare symbol array with nowhere to put a default. An application that set DEEPL_AUTH_KEY and never assigned config.deepl_api_key worked on main and raised ConfigurationError on the first translate here -- at boot, for a Rails app. configuration_options now takes either a bare symbol or `key => default`, and a default routes through Configuration.option's existing callable support, so it is resolved on read rather than at load: the variable may be exported after this gem is required. A blank default reads as unset, the rule assignment already followed, so DEEPL_AUTH_KEY= is a missing key rather than an empty credential. Amazon declares no fallback on purpose and now says so. CHANGELOG: the sentence claiming Google reads those variables and falls back to application default credentials was false twice over -- corrected, ADC recorded as dropped, and the two missing Breaking entries added (deepl_host -> deepl_api_base, and a count mismatch raising ResponseError rather than Request::Error).
Providers.register raised InvalidProviderError for a class that skipped the base class, but config.provider = <object> -- a documented extension point -- returned any non-Symbol untouched, so an object was never checked. An app that upgraded with its own duck-typed provider object heard nothing at configure time and got NoMethodError: undefined method 'capabilities' from request.rb on the first translate, with no message naming the contract change. The `provider:` keyword had the same hole. All three paths now raise InvalidProviderError in the same words. cache, segmenter and rate_limiter are still genuinely duck-typed and stay untouched, which a test now pins. Also fixes the registry guard itself (M8): `klass < Provider` raised NoMethodError for an instance, nil or a symbol, and ArgumentError for a non-Module -- the first thing someone writing their own provider hits. The message names the class, never the object, whose #to_s would render its own contents.
Only Google and DeepL normalised. Measured with from: "EN", to: "RU", Amazon sent "SourceLanguageCode" => "EN", LibreTranslate and ModernMT sent "EN"/"RU", and Azure sent &to=RU&from=EN. Amazon Translate rejects those outright and LibreTranslate answers 400 -- on every call, for anyone who followed the README's "switch provider by changing config.provider" while keeping DeepL-style upper-case codes. The rule Google had is now Provider#language, reachable by the base class and by every provider, with self.language_case choosing the casing: DeepL up-cases, the other five down-case. The subtag exception is kept and now applies to DeepL too, which up-cased "zh-Hans" into "ZH-HANS" -- the casing of a script or region subtag is its own and a blanket transform corrupts it. Tested per provider, both the bare code and the subtag, not only Google.
…t it deepl.rb and modernmt.rb mapped a summed 0 to nil while azure.rb returned 0 when its header said 0, so the same fact reached a caller two ways depending on the provider. nil now means the provider reported nothing and a number, 0 included, means it reported that number -- Azure's behaviour, and the one Translation::Usage's own comment already described. A provider that bills nothing for a call (a translation memory hit) no longer has that reported as "unknown". The rule lives once, in Provider#billed_characters, rather than twice.
amazon.rb applied its mandatory fields first and the caller's options second, so TranslationDiff.translate(text, to: :ru, TargetLanguageCode: "de") sent German, and Text: could substitute somebody else's text for the caller's. Azure had the same shape, reachable by anyone building a Translation::Request directly. All six now apply defaults, then caller options, then mandatory fields, which is what the other four already did: a caller can still override a default such as Azure's textType, and can no longer displace the language pair or the texts.
M1 changed Cache#store and I1 changed how provider options are declared; either moving a key would make every user re-translate their whole corpus on upgrade. This pins five keys covering the provider segment, the normalised language codes including a subtag, the options digest over a nested hash and an array, and the sentence digest. The five values were read off `main` and are byte-identical there.
The matrix still listed 3.2 and 3.3, which this branch dropped when it raised `required_ruby_version` to 3.4. Both jobs failed at `bundle install`, because bundler correctly refuses to install a gem that declares a floor above the running Ruby -- so the failure was the gemspec being obeyed, not the code being wrong. A matrix that outlives the version it tests turns CI red for a reason unrelated to any change, which is the fastest way to teach a team to merge over a red build.
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.
Summary
This branch replaces the provider layer end to end and, with this last
commit, connects it back to
TranslationDiff.translate-- before thiscommit the pipeline still spoke the pre-refactor duck-typed contract and
every translation call raised
NoMethodError.through
deepl-rb/google-cloud-translate-v2. Every REST-backedprovider now owns one Faraday connection (
TranslationDiff::HTTPProvider),with its own retry policy, timeouts, and error mapping -- and, unlike the
SDKs it replaced, installs no logging middleware and accepts no way to
turn one on, so no HTTP client this library builds can ever be handed
source text, a translation, or a credential to log.
faradayandfaraday-retrybecome runtime dependencies;deepl-rbandgoogle-cloud-translate-v2are gone.Providerbase class.TranslationDiff::Providersupplies theconfiguration check, capability defaults, and
#cache_key; a provider nolonger merely happens to answer the right methods, it inherits the
contract.
TranslationDiff::Providers.registernow refuses anything thatdoesn't inherit it.
max_request_sizeand
max_batch_sizeused to be provider methods (and "can it detect alanguage" was
respond_to?(:detect), which every provider now answerstruthfully regardless). They're
TranslationDiff::Capabilitiesnow:request/batch/text size limits, which option turns HTML handling on (every
vendor spells it differently), whether
notranslateis honoured, whetherdetection is supported, and whether billed-character usage is reported --
one place per provider, checked by the pipeline instead of assumed.
:deepl(default) -- unchanged behaviour, now via direct HTTP; thebatch limit was corrected from a wrong 300 to DeepL's documented 50.
:google-- unchanged behaviour, now via direct HTTP, no moregoogleauth/signet/grpc pulled in for one POST with a key in the query
string.
:azure(new) -- Azure AI Translator, the most generous per-requestlimits of the set and the only one besides DeepL that reports billed
characters.
:modernmt(new) -- adaptive MT with translation memories.:libretranslate(new) -- the one self-hostable provider. Measuredagainst a real instance: its HTML mode preserves
notranslatemarkupbut translates the protected content anyway, so
Capabilitiesdeclaresnotranslate: falserather than trusting the vendor's own HTML claim.:amazon(new) -- signed withaws-sigv4(required lazily, only bythis provider) rather than merely headed. No batch API (one text per
call) and no HTML mode at all, both stated as capabilities rather than
discovered by a customer.
Translation::Request/Translation::Responsereplace the oldtranslate(texts, from:, to:, **options)/ bare array. A provider takesone, returns the other;
Response.buildis the one place the returnedcount is checked (
ResponseErroron a mismatch), so that check now holdsfor every provider, including ones (Amazon) that override
#translateoutright instead of using the HTTP seams.
ConfigurationError,ProviderErrorand itsAuthenticationError/RateLimitError/QuotaExceededError/InvalidRequestError/ServiceError,TransportError,ResponseError,InvalidProviderError) replaces bareTranslationDiff::Errorforanything a provider's transport can do wrong.
Request#chunks,#detect_languageand#call_apionto capabilities and the new request/response types (thethree places nothing else in the branch had touched), migrates the three
test files that were still carrying pre-
Providerdoubles forNull(
instrumentation_test.rb,context_test.rb,request_test.rb) onto thereal contract, and fixes a real aliasing bug the required chunking test
surfaced: a provider handing back the same array it was given (rather
than a fresh one, as
:nulldoes) had that array silently drained toempty by
Cache#store's destructive#shift--call_apinow dups itsreturn value.
provider table (every number cross-checked against
TranslationDiff::Capabilitiesdirectly, not read off the source by eye),a "Writing a provider" guide with a full working example on the three
HTTP seams, and the Amazon/LibreTranslate
notranslatecaveats called outin prose so a reader sees them before their bill and their brand names
arrive, not after.
Verification
bundle exec rake testgreen on seeds 1, 7, 99, 1234, 4242 (343 runs,783 assertions, 0 failures).
bundle exec rubocop: no offenses.TranslationDiff::Providers.names=>[:null, :deepl, :google, :azure, :modernmt, :libretranslate, :amazon].mainand through this branch with the
:nullprovider -- byte-identicaloutput.
Test plan
bundle exec rake teston a clean checkoutbundle exec rubocopCapabilitiesfor theprovider you know best
Cache#keyinlib/translation_diff/cache.rbis untouched by this branch)Not merging -- left for review.