feat: what a real application needed and the gem did not give it - #25
Merged
Conversation
Google mangled `jq '.meters'` into `jq '.metros'` inside a live <pre><code> block because nothing told the pipeline that code holds language, not prose. pre and code now join script and style in Passage::Scanner's opaque set, and the set is a plain config.opaque_elements option (default: script, style, pre, code) so an application can widen or narrow it. While testing the nesting case (a notranslate span inside an opaque element), found and fixed a real bug: closing a protected element that itself raised @opaque_depth (either by name, like <code class="notranslate">, or by ancestry, like a notranslate span inside <code>) never gave that increment back, because a protected element's own end_element takes the early-return branch and never reaches the opaque decrement. Every sentence after such a block silently stopped being sent for translation. attr now undoes the bump right where it made it. Metrics/ClassLength bumped from 100 to 110 for lib/**/* -- Configuration is a flat table of declared options, and this one is a genuine addition, not untidiness.
A subscriber receiving cache, request and usage events had no way to tell which translate call they belonged to short of tagging Thread.current themselves -- a workaround that breaks the moment two translations share a thread. Translator now generates one opaque call_id per call and puts it in translate, cache, request, rate_limit, usage and cache_error; Dispatcher receives it rather than making its own, since it emits three of the six.
request and usage only fire when a provider is reached, so a call served entirely from cache left a subscriber with nil where it wanted a count. translate now carries characters: every non-blank segment this call looked at, hit or miss, computed once regardless of how the cache split it into batches. request's own characters keeps its narrower meaning -- what one batch actually sent -- distinguished by which event it's on.
Configuration memoised provider_instance, cache_store, segmenter_instance, rate_limiter_instance and redis_pool, and nothing ever cleared them. An application wanting to switch provider at runtime had no way to do it short of TranslationDiff.reset! and reconfiguring from scratch, which also threw away the cache store and the Redis pool it had no reason to touch. Each option declaration now names, via `option ... invalidates:`, exactly which memoised reader(s) it feeds; the writer `option` generates clears only those ivars. Provider options get `invalidates: :provider_instance` automatically from register_provider_options, so a provider's own option names never need listing by hand. An option nobody classifies invalidates nothing, the same as logger, instrumenter and the timeouts. cache_ttl=, cache_namespace= and cache_prune_probability= are defined on prepended modules and were setting their ivars directly, bypassing the generic writer entirely; they now delegate to it through `super` so their invalidation runs too.
… option table The previous change raised the global ClassLength ceiling from 100 to 110 just to let Configuration fit, which makes the limit meaningless for every other class. Put it back to 100. Configuration's bulk was a flat table of `option` declarations -- exactly the kind of thing that belongs in its own module rather than in the class that implements the behaviour. Moved it to TranslationDiff::Configuration::OptionTable, a plain [key, default, invalidates] table with one line per option, applied to the class with `OptionTable.declare_on(self)`. Configuration now fits under 100 lines without an exclude or a raised ceiling.
… hits Reuses translate's own Document/Passage segmenter, SentenceCache key, and provider resolution rather than reimplementing them, so it cannot drift from what translate actually does. Refuses cleanly when from: is nil and the provider would need a paid detection request to answer.
Previewer had copied Translator's private provider-resolution branch verbatim because it was private, leaving two copies of "which provider does this call use" free to drift. Move it to TranslationDiff::Providers, which already owns registration and the build/ensure_provider! guards, as TranslationDiff::Providers.resolve -- the cache_key guard travels with it, since a blank cache_key is a provider-validity failure, not a translator- or previewer-specific one. Its raised error changes from Translator::Error/Previewer::Error to the more precise InvalidProviderError. This also drops Translator under Metrics/ClassLength's 100-line limit (104 -> under 100).
Comments in this codebase stay at most one line; the extraction commit left a few spanning two or three.
RedisRateLimiter.build and ActiveRecordRateLimiter.build both take their namespace from config.cache_namespace, but the per-option invalidation only mapped cache_namespace to cache_store. Changing the namespace at runtime left a memoised rate_limiter_instance counting under the old one, silently, and only in the Redis case (the ActiveRecord limiter reads the DB row's namespace column fresh on every check). The mapping was wrong, not the limiter's choice of option: the limiter reading cache_namespace is the existing, intended behaviour (see Configuration#copy's own comment on it), so cache_namespace now also invalidates rate_limiter_instance. This is a behaviour change for anyone who set cache_namespace after first touching the limiter, expecting the old (buggy) memoised instance to stick: it now moves too.
…dation and preview Five behaviours from building a real Rails + Hotwire app on this gem, verified against the code and (where practical) live providers: call_id on every event, translate's own characters, pre/code joining the opaque element set, options invalidating only the collaborator they feed, and TranslationDiff.preview.
Ox hands back element names exactly as written (:PRE, :Pre, :STYLE), never lowercased, so Scanner#start_element compared a downcased opaque set against an un-downcased name and uppercase/mixed-case pre, code, script and style all leaked through to the provider. Fix the comparison and the comment above protection? that had the same claim backwards.
translate and preview are a matched pair at the top level, but Context only carried translate, so a tenant's call could never be previewed. Add preview, built the same way translate is, against the context's own configuration.
…ewer Both passed segmenter: and language: from the configuration a call is actually using, but not opaque_elements, so Passage fell back to the global TranslationDiff.config and a context's own opaque_elements was silently ignored -- the one mechanism the docs recommend for a per-tenant setting. Pass it through in both places.
…iewer Previewer and Translator carried verbatim copies of passage, same_language?, ensure_supported!, the "cannot detect the source language" refusal and the SentenceCache construction -- the exact duplication that let the opaque_elements argument be forgotten in both places. Extract the shared parts into CallPreparation, included by both, so the next omission cannot happen twice. Each class keeps its own Error and does not merge into the other.
open_timeout, timeout and max_retries were classified as invalidating nothing, but HTTPProvider#connection memoises a Faraday connection built from all three, and the provider itself is memoised on Configuration. Raising config.timeout from a settings screen changed the value and nothing else until the process restarted. logger and instrumenter really are read live and stay classified as they are.
Every write cleared its declared memos even when the value was
unchanged, so a per-request `configure { |c| c.cache_namespace =
tenant }` rebuilt the cache store on every request -- on the default
MemoryCacheStore a rebuild is a brand new empty Hash, so the
application paid the provider for its whole warm cache again. Compare
the raw stored value before writing and invalidating.
This does not cover every case: changing cache_ttl on a memory store
still drops a cache that never read that option, since cache_ttl is
still classified as invalidating cache_store.
Preview reported sendable_characters -- the numerator -- with no denominator, on the same branch that added characters to the translate event because a fully-cached call could not report its size. Add characters, the same total translate reports, and test that the two agree for the same call: the strongest available check.
…o-op-write fixes Opaque element matching is now documented as case-insensitive; a context's own config.opaque_elements is documented as reaching Passage instead of falling back to the global value; the timeouts are corrected from "rebuild nothing" to "rebuild the provider"; a same- value write is documented as invalidating nothing, and what a genuinely new cache option value still costs on MemoryCacheStore; Context is documented as offering #preview alongside #translate; Preview#characters is documented and tied back to the cache and translate events it reconciles against. Also fixes three standalone documentation defects: the instrumentation event table contradicted its own page about translate/cache firing on a fully-cached call; usage's characters was left out of the "two events, different meaning" characters note; and two upgrade consequences were filed as Added sub-bullets instead of under Breaking, where a blank provider cache_key now raising InvalidProviderError instead of Translator::Error has been added alongside them. README now shows TranslationDiff.preview in its code section.
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.
Five things a real application needed and this gem did not give it, found by
building a Rails + Hotwire bench on top of it and translating a real HTML
article through live providers. Each one cost that application a workaround.
What the bench could not do
Correlate events. No payload said which
translateacachebelongedto, so the bench tagged
Thread.currentin its job and read it back in thesubscriber — which breaks the moment two translations share a thread. Every
event from one call now carries the same
call_id.Report a fully cached run.
requestandusagefire only when aprovider is reached, so a run served entirely from cache left a subscriber
holding
nilwhere it wanted a number.translatenow carriescharacters: what the call considered, sent or not.requestkeeps itsnarrower meaning — what one batch sent.
Ask what a call would cost. The bench's "this edit will send 1 sentence"
preview had to read
Segment,FragmentandSentenceCacheinternals andreimplement the cache key.
TranslationDiff.previewanswers it properly —same segmenter, same key, same digest, no provider call, no write — and a
test pins its numbers to what the
cacheevent then reports, so it cannotdrift from the thing it predicts.
Change a setting at runtime. Every collaborator was memoised and nothing
invalidated, so switching provider meant
reset!and rebuilding the wholeconfiguration, discarding a Redis pool that had no reason to go. A write now
invalidates exactly what it affects, and a write that changes nothing
invalidates nothing.
What the article exposed
Code was translated as prose. Google turned
jq '.meters'intojq '.metros'inside a<pre><code>block, because nothing told thepipeline that code is not language.
preandcodejoinscriptandstyle, the set is nowconfig.opaque_elements, and the match iscase-insensitive —
<PRE>,<CODE>and<STYLE>are what Word, Outlookand older CMSes emit, and an uppercase
<STYLE>was coming back translatedinto Spanish.
A latent bug, found while testing the above. A
notranslatespan insidean opaque element left the walker's depth counter one too high, and every
sentence after such a block silently stopped being sent for translation.
Upgrade consequences
Filed under Breaking in the CHANGELOG, not buried under Added:
preorcode— what is sentchanged, so what is keyed changed;
cache_namespaceoractive_record_basechange now moves therate limiter as well as the store, instead of leaving the limiter counting
under the old namespace;
cache_keyraisesTranslationDiff::InvalidProviderErrorrather thanTranslationDiff::Translator::Error— a sibling class, not a subclass.And one honest remainder, documented where somebody tuning configuration will
read it: changing a cache option to a genuinely new value still rebuilds the
store, and if that store is the in-process memory one, its contents are gone.
Review found, and this branch fixed
The first pass at the opaque set matched case-sensitively,
opaque_elementswas read only from the global configuration so a per-tenant
Contextwasignored, the timeouts were classified as invalidating nothing while a
memoised provider builds its connection from them, and a no-op write still
cleared the memo — measurably re-billing a warm memory cache.
Translatorand
Previeweralso still carried five verbatim copies of shared behaviour,which is exactly why the missing
opaque_elements:argument had to beforgotten twice, and was; they now share
CallPreparation.745 runs on SQLite, 753 against real PostgreSQL, rubocop clean. Verified live
against Google and DeepL.