3daa0c34db
- getEffectiveFormality() now checks *TranslationContext for formality - Priority: TranslationContext > Subject > map["Formality"] > Service - Add tests for context formality override behavior - Update FUTURE_PROOFING.md: Context Integration now IMPLEMENTED - Update REVIEW.md: Context wiring recommendation addressed Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
38 lines
No EOL
2.6 KiB
Markdown
38 lines
No EOL
2.6 KiB
Markdown
# Code Review: i18n Package (Refactored)
|
|
|
|
## Executive Summary
|
|
The `pkg/i18n` package has undergone a significant refactoring that addresses previous architectural concerns. The introduction of a `Loader` interface and a `KeyHandler` middleware chain has transformed a monolithic service into a modular, extensible system. The file structure is now logical and intuitive.
|
|
|
|
## Status: Excellent
|
|
The package is now in a state that strongly supports future growth without breaking changes. The code is clean, idiomatic, and follows Go best practices.
|
|
|
|
## Improvements Verified
|
|
|
|
* **Modular Architecture:** The "magic" namespace logic (e.g., `i18n.label.*`) has been successfully extracted from the core `T()` method into a chain of `KeyHandler` implementations (`handler.go`). This allows for easy extension or removal of these features.
|
|
* **Storage Agnosticism:** The new `Loader` interface and `NewWithLoader` constructor decouple the service from the filesystem, allowing for future backends (Database, API, etc.) without API breakage.
|
|
* **Logical File Structure:**
|
|
* `service.go`: Core service logic (moved from `interfaces.go`).
|
|
* `loader.go`: Data loading and flattening (renamed from `mutate.go`).
|
|
* `hooks.go`: Callback logic (renamed from `actions.go`).
|
|
* `handler.go`: Middleware logic (new).
|
|
* `types.go`: Shared interfaces and types (new).
|
|
* **Type Safety:** Shared types (`Mode`, `Formality`, etc.) are centralized in `types.go`, improving discoverability.
|
|
|
|
## Remaining/New Observations
|
|
|
|
| Issue | Severity | Location | Recommendation |
|
|
|-------|----------|----------|----------------|
|
|
| **Context Integration** | Minor | `service.go` | `TranslationContext` is defined in `context.go` but not yet fully utilized in `resolveWithFallback`. The service checks `Subject` for formality, but doesn't appear to check `TranslationContext` yet. |
|
|
| **Handler Performance** | Trivial | `handler.go` | The handler chain is iterated for every `T()` call. For high-performance hot loops, ensure the chain length remains reasonable (current default of 6 is fine). |
|
|
|
|
## Recommendations
|
|
|
|
1. **Wire up Context:**
|
|
* Update `Service.getEffectiveFormality` (and similar helper methods) to check for `*TranslationContext` in addition to `*Subject`.
|
|
* This will fully activate the features defined in `context.go`.
|
|
|
|
2. **Unit Tests:**
|
|
* Ensure the new `handler.go` and `loader.go` have dedicated test coverage (files `handler_test.go` and `loader_test.go` exist, which is good).
|
|
|
|
3. **Documentation:**
|
|
* Update package-level examples in `i18n.go` to show how to use `WithHandlers` or custom Loaders if advanced usage is expected. |