refactor: group lib the way the concepts group - #26
Merged
Conversation
MemoryCacheStore, RedisCacheStore and ActiveRecordCacheStore become TranslationDiff::Stores::Memory/Redis/ActiveRecord, living beside the Stores registry the way ruby_llm nests providers under Providers. Stores.register(:redis, Stores::Redis) now reads the way it means. Stores::Redis qualifies Redis::Namespace with :: since it now shares a namespace with the class of the same name; also rewires lib/translation_diff.rb's require order for the whole cache/limiter/ active_record/configuration reorganisation landing across this and the next few commits.
RedisRateLimiter and ActiveRecordRateLimiter become TranslationDiff::RateLimiters::Redis/ActiveRecord, matching Stores' new shape and the registry name each is registered under.
ActiveRecordSupport becomes TranslationDiff::ActiveRecord::Support, the mixin the cache store and rate limiter both include. Since TranslationDiff::ActiveRecord now shares its name with ::ActiveRecord, the two references it makes to the real gem across a genuinely missing constant (ar_error?'s defined?/is_a? pair) are :: qualified; the version floor check and the anonymous model's base class already were.
CacheTtlOption and CacheGuardOptions become TranslationDiff::Configuration::CacheTtlOption/CacheGuardOptions, prepended pieces of Configuration rather than loose top-level modules -- the same shape option_table.rb already had. Their requires move inside Configuration's own class body, next to option_table's, since the modules they define now nest under a class that has to exist first.
…move MemoryCacheStore, RedisCacheStore, ActiveRecordCacheStore, RedisRateLimiter, ActiveRecordRateLimiter and ActiveRecordSupport are gone; every mention across README, docs/ and the Gemfile's dependency comments now names the class actually in lib/. Adds an Unreleased CHANGELOG entry naming the renames, since the source now shows them even though behaviour didn't change.
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.
lib/translation_diff/had thirty files at its top level, mixing thepipeline's domain objects with cache stores, rate limiters and pieces of
configuration.
ruby_llm, the architecture this project follows, uses onerule without exception: a file names a concept, a directory of the same name
holds its implementations. Our
providers/,segmenters/,languages/andtranslation/already had that shape; the rest did not.Behaviour is unchanged. No back-compatibility aliases — the gem is not
really released yet, so the old constants are simply gone.
MemoryCacheStore,RedisCacheStore,ActiveRecordCacheStoreStores::Memory,Stores::Redis,Stores::ActiveRecordRedisRateLimiter,ActiveRecordRateLimiterRateLimiters::Redis,RateLimiters::ActiveRecordActiveRecordSupportActiveRecord::SupportCacheTtlOption,CacheGuardOptionsConfiguration::Class names now match the names they are registered as:
Stores.register(:redis, Stores::Redis). The registry names themselves —:memory,:redis,:active_record, the six providers — are untouched,since they are what applications actually configure.
The pipeline's domain objects stay at the top level.
ruby_llmkeeps its owndomain there for the same reason, and moving them would be rearranging for
its own sake.
The risk, which is the only interesting part
Stores::Redisshadows::RedisandStores::ActiveRecordshadows::ActiveRecordfor every constant lookup nested inside them: an unqualifiedRedis::Namespacewould resolve to the gem's own class and fail somewhereunhelpful. Every reference to the real gems is
::-qualified, and everyclass is declared in compact form (
class TranslationDiff::Stores::Redis),which keeps
Module.nestingto one entry and makes the shadowingstructurally impossible rather than merely avoided.
That was verified where it can actually fail: against a real
redis-serverand a real PostgreSQL, not the suite's fakes — every Redis test in this
project runs against a double, so a green suite proves nothing here.
Also
order; it now lives with the rest.
TranslationDiff::RateLimitExceeded. The two limiters used toraise two same-named classes under their own namespaces, so rescuing one
and switching
rate_limiterto the other quietly stopped catching it.745 runs on SQLite, 753 against real PostgreSQL, rubocop clean. Cache keys
byte-identical to
main, and nothing eager-loads:require "translation_diff"still leaves
ActiveRecordandRedisundefined with those gems strippedfrom the load path.