feat: add i18n support for 5 languages (en, es, fr, de, zh) #593
Conversation
RIVALHIDE
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (6)
✏️ Tip: You can disable in-progress messages by setting Tip Migrating from UI to YAML configuration.Use the 📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @RIVALHIDE, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Cortex CLI by integrating full internationalization capabilities. It allows the CLI to adapt to the user's language preference, providing a localized experience across all messages and data formatting. This change broadens the accessibility and usability of Cortex for a global user base, making it more intuitive and user-friendly regardless of their native language. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-structured internationalization (i18n) framework, which is a fantastic addition to the CLI. The implementation is thorough, covering translation, configuration, auto-detection, formatting, and testing. The new documentation and test suite are particularly impressive. My review focuses on a few areas for improvement to enhance maintainability and performance:
- Resolving a circular dependency within the
i18nmodule to improve the overall design and remove code duplication. - Optimizing a language name resolution function for better performance.
- Suggesting a more direct use of the
babellibrary for formatting to simplify the code and improve scalability.
Overall, this is a very strong contribution that significantly enhances the user experience.
I am having trouble creating individual review comments. Click here to see my feedback.
cortex/i18n/config.py (20-21)
This SUPPORTED_LANGUAGES constant is a duplicate of the one defined in cortex/i18n/translator.py. This duplication can lead to maintenance issues if the two get out of sync. It appears to be a workaround for a circular dependency between config.py and translator.py.
To resolve this, I recommend creating a single source of truth in a new file, for example cortex/i18n/constants.py. Both modules could then import from this new file, which would break the dependency cycle and eliminate the duplicated constant.
Example for cortex/i18n/constants.py:
SUPPORTED_LANGUAGES: dict[str, dict[str, str]] = {
"en": {"name": "English", "native": "English"},
# ... other languages
}
DEFAULT_LANGUAGE = "en"This change would significantly improve the design and robustness of the i18n module.
cortex/cli.py (978-981)
This local import (from cortex.i18n.translator import reset_translator) is used here and in other places to work around a circular dependency. While this pattern works, it's often a sign of a design issue that could be improved.
Resolving the underlying circular dependency (as suggested in another comment regarding cortex/i18n/config.py) would allow this import to be moved to the top of the file, improving code readability and structure.
lang_config.clear_language()
reset_translator()
cortex/cli.py (2140-2178)
The _resolve_language_name function rebuilds the name_to_code lookup dictionary on every call. Since the list of supported languages is static, this dictionary can be created once and cached for better performance.
You can achieve this by attaching the lookup table to the function object itself, so it's only computed on the first call. This avoids polluting the module's global namespace while still being efficient.
def _resolve_language_name(name: str) -> str | None:
"""
Resolve a language name or code to a supported language code.
Accepts:
- Language codes: en, es, fr, de, zh
- English names: English, Spanish, French, German, Chinese
- Native names: Español, Français, Deutsch, 中文
Args:
name: Language name or code (case-insensitive for Latin scripts)
Returns:
Language code if found, None otherwise
"""
if not hasattr(_resolve_language_name, "_lookup"):
lookup = {}
for code, info in SUPPORTED_LANGUAGES.items():
lookup[code] = code
lookup[info["name"].lower()] = code
lookup[info["native"].lower()] = code
lookup[info["native"]] = code # For case-sensitive names like 中文
_resolve_language_name._lookup = lookup
stripped_name = name.strip()
# Check lowercased version first, then original for case-sensitive names
return _resolve_language_name._lookup.get(stripped_name.lower()) or _resolve_language_name._lookup.get(stripped_name)cortex/i18n/formatter.py (148-153)
The LocaleFormatter class provides a custom implementation for formatting dates, numbers, and other data types. While functional, this approach is difficult to maintain and scale, as it requires adding large configuration blocks to LOCALE_CONFIGS for each new language.
The pull request description mentions using Babel. I recommend refactoring this class to use the babel library's formatting functions directly. This would leverage its extensive locale data, simplify the code, and make it more robust.
For example, format_date could be simplified to:
from babel.dates import format_date as babel_format_date
def format_date(self, dt: datetime) -> str:
"""Formats a date using babel."""
return babel_format_date(dt, locale=self.language)Similar refactoring can be applied to format_number, format_datetime, and format_time_ago, which would remove the need for the large LOCALE_CONFIGS dictionary.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/i18n/config.py`:
- Around line 54-60: The TOCTOU arises because the existence check for
self.preferences_file happens before acquiring self._file_lock; move the
exists() check inside the critical section so the lock covers both the check and
the file read in _load_preferences (acquire self._file_lock, then if
self.preferences_file.exists(): open/read with yaml.safe_load and return or {}),
and keep the existing exception handling (yaml.YAMLError, OSError) to handle
race-related failures.
In `@cortex/i18n/formatter.py`:
- Around line 358-370: get_formatter currently caches a LocaleFormatter once and
never updates it when the app language changes; modify get_formatter so that
after importing and calling get_language() it compares that language to the
existing _formatter's language and, if different, either set _formatter.language
to the new value (if LocaleFormatter exposes a setter) or re-create _formatter =
LocaleFormatter(language=new_lang); ensure this logic lives inside get_formatter
so every access stays in sync and also handle the case where _formatter is None
by creating it as before.
🧹 Nitpick comments (8)
docs/i18n-guide.md (1)
188-195: Add language specifier to fenced code block.The code block showing the directory structure is missing a language specifier, which triggers a markdown lint warning (MD040). Use
textorplaintextfor non-code content.🛠️ Suggested fix
-``` +```text cortex/i18n/locales/ ├── en.yaml # English (source language) ├── es.yaml # Spanish ├── fr.yaml # French ├── de.yaml # German └── zh.yaml # Chinese</details> </blockquote></details> <details> <summary>tests/test_i18n.py (3)</summary><blockquote> `649-659`: **Path construction may be fragile across different test execution contexts.** The path construction at line 655 assumes a specific relative directory structure from the test file location. This could break if tests are run from different working directories or if the project structure changes. <details> <summary>♻️ Suggested improvement using package resource discovery</summary> ```diff def test_all_supported_languages_have_catalogs(self): """Test that all supported languages have message catalogs.""" - from pathlib import Path - from cortex.i18n.translator import SUPPORTED_LANGUAGES + import importlib.resources - locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" + # Use importlib.resources for reliable package resource discovery + try: + # Python 3.9+ + locales_ref = importlib.resources.files("cortex.i18n") / "locales" + except AttributeError: + # Fallback for older Python + from pathlib import Path + locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" + else: + locales_dir = locales_ref for lang_code in SUPPORTED_LANGUAGES: - catalog_path = locales_dir / f"{lang_code}.yaml" - self.assertTrue(catalog_path.exists(), f"Missing catalog for language: {lang_code}") + # Check if catalog exists + catalog_name = f"{lang_code}.yaml" + if hasattr(locales_dir, 'joinpath'): + catalog_path = locales_dir.joinpath(catalog_name) + exists = catalog_path.is_file() if hasattr(catalog_path, 'is_file') else True + else: + catalog_path = locales_dir / catalog_name + exists = catalog_path.exists() + self.assertTrue(exists, f"Missing catalog for language: {lang_code}")Alternatively, you could verify catalog existence by attempting to load them through the Translator class, which already handles resource discovery internally.
20-21: Unused import:yaml.The
yamlmodule is imported but never used in the test file. This was likely intended for direct YAML parsing but the tests use the Translator class instead.🧹 Remove unused import
from unittest.mock import MagicMock, patch -import yaml - class TestTranslator(unittest.TestCase):
18-18: Unused import:MagicMock.
MagicMockis imported but not used in this test file. Onlypatchis used fromunittest.mock.🧹 Remove unused import
-from unittest.mock import MagicMock, patch +from unittest.mock import patchcortex/i18n/config.py (1)
19-21: Consider renaming to avoid confusion withtranslator.SUPPORTED_LANGUAGES.The comment acknowledges the circular import concern, but using the same name for different types (set here vs dict in translator.py) can confuse maintainers. Consider renaming to
SUPPORTED_LANGUAGE_CODESto clarify it's just for membership checking.cortex/cli.py (1)
1164-1177: Consider narrowing the exception type.The catch-all
Exceptionon line 1175 masks the specific error. Sinceset_languageraisesValueErrorand_save_preferencesraisesRuntimeError, catching these specifically would provide better error handling.♻️ Proposed refinement
try: lang_config.set_language(code) # Reset the global translator to pick up the new language from cortex.i18n.translator import reset_translator reset_translator() set_language(code) lang_info = SUPPORTED_LANGUAGES[code] cx_print(t("language.changed", language=lang_info["native"]), "success") return 0 - except Exception as e: + except (ValueError, RuntimeError) as e: self._print_error(f"Failed to set language: {e}") return 1cortex/i18n/formatter.py (1)
330-351: Duration format strings are not localized.The method uses hardcoded English abbreviations ("ms", "s", "m", "h"). While this is acceptable for a CLI tool (these abbreviations are widely understood), consider adding localized duration strings to
LOCALE_CONFIGSfor full i18n consistency if needed in the future.cortex/i18n/translator.py (1)
173-180: Consider catching additional interpolation errors.The
str.format()method can raiseIndexErrorfor positional arguments andValueErrorfor malformed format strings. WhileKeyErrorhandles named arguments, broader exception handling would be safer.♻️ Proposed refinement
# Interpolate variables if kwargs: try: message = message.format(**kwargs) - except KeyError: + except (KeyError, IndexError, ValueError): # If interpolation fails, return message without interpolation pass
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cortex/cli.pycortex/i18n/__init__.pycortex/i18n/config.pycortex/i18n/detector.pycortex/i18n/formatter.pycortex/i18n/locales/de.yamlcortex/i18n/locales/en.yamlcortex/i18n/locales/es.yamlcortex/i18n/locales/fr.yamlcortex/i18n/locales/zh.yamlcortex/i18n/translator.pydocs/i18n-guide.mdpyproject.tomltests/test_i18n.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/i18n/detector.pycortex/i18n/config.pycortex/i18n/__init__.pytests/test_i18n.pycortex/cli.pycortex/i18n/formatter.pycortex/i18n/translator.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_i18n.py
🧬 Code graph analysis (3)
cortex/i18n/config.py (2)
cortex/i18n/detector.py (1)
detect_os_language(104-144)cortex/i18n/translator.py (5)
get_language(279-286)set_language(265-276)language(61-63)language(66-83)get_language_info(318-334)
cortex/i18n/formatter.py (3)
cortex/i18n/translator.py (3)
language(61-63)language(66-83)get_language(279-286)cortex/cli.py (1)
config(1080-1094)cortex/i18n/config.py (1)
get_language(77-107)
cortex/i18n/translator.py (2)
cortex/i18n/formatter.py (2)
language(165-167)language(170-175)cortex/i18n/config.py (3)
get_language(77-107)set_language(109-128)get_language_info(139-176)
🪛 markdownlint-cli2 (0.18.1)
docs/i18n-guide.md
188-188: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (29)
cortex/i18n/locales/zh.yaml (1)
1-307: LGTM! Comprehensive Chinese translations.The Chinese (Simplified) locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations cover all required sections for CLI i18n support.
pyproject.toml (2)
2-2: Setuptools version bump looks appropriate.The bump to
setuptools>=64.0provides better support forpackage-dataconfiguration. This version has been stable since 2022.
101-106: Package configuration correctly includes i18n module and locale data.The
cortex.i18npackage is properly added to the packages list, and thepackage-dataentry ensures YAML locale files are included in distributions. This enables runtime loading of translation catalogs.cortex/i18n/detector.py (4)
14-52: Well-structured language mappings with comprehensive variant coverage.The
SUPPORTED_LANGUAGESset andLANGUAGE_MAPPINGSdictionary properly cover regional variants for all five supported languages. The C/POSIX locale defaults to English, which follows standard POSIX behavior.
55-101: Robust locale parsing with comprehensive fallback strategies.The
_parse_localefunction handles multiple locale string formats well (e.g.,en_US.UTF-8,es_ES,zh-CN). The progressive fallback through direct mapping, language prefix, and full locale lookup ensures maximum compatibility.Type hints and docstring are present per coding guidelines.
104-144: Correct implementation of OS language detection.The function follows the standard GNU gettext priority order (
LANGUAGE→LC_ALL→LC_MESSAGES→LANG) and correctly handles the colon-separatedLANGUAGEvariable. The fallback to English ensures graceful degradation.Docstring with examples and type hints are present per coding guidelines.
147-162: Useful debugging utility for locale information.The
get_os_locale_infofunction provides comprehensive locale environment variable exposure plus the computeddetected_language, which is valuable for troubleshooting i18n issues.cortex/i18n/locales/de.yaml (1)
1-307: LGTM! Comprehensive German translations.The German locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations appear natural and cover all required CLI sections.
cortex/i18n/locales/es.yaml (1)
1-307: LGTM! Comprehensive Spanish translations.The Spanish locale file is well-structured, mirrors the English source catalog, and preserves all placeholders for dynamic value interpolation. The translations cover all required CLI sections for complete i18n support.
cortex/i18n/locales/en.yaml (1)
1-375: Well-documented source catalog with excellent translator guidance.The English source file is well-structured with:
- Clear variable documentation comments above interpolated messages (e.g., lines 35-38, 44-45)
- Consistent snake_case key naming
- Logical grouping by feature/module
- Comprehensive coverage of all CLI messages
This serves as an excellent reference for translators.
docs/i18n-guide.md (1)
1-480: Comprehensive and well-organized i18n documentation.The guide thoroughly covers all aspects of the i18n system including usage examples, adding new languages, translation guidelines, and troubleshooting. The language resolution order documentation (lines 422-429) is particularly valuable for understanding priority.
cortex/i18n/__init__.py (1)
1-58: Clean public API surface with comprehensive documentation.The module initialization properly:
- Provides a centralized entry point for all i18n functionality
- Documents usage examples in the module docstring (lines 10-20)
- Organizes
__all__exports by category for clarity- Lists all supported languages in the docstring
tests/test_i18n.py (1)
1-977: Comprehensive test suite with excellent coverage of i18n components.The test suite covers:
- Core translator functionality including language switching, interpolation, and fallback
- Configuration persistence with proper temp directory isolation
- OS language detection with various environment variable combinations
- Locale-aware formatting across all supported languages
- CLI integration for language commands
- Language name resolution for both codes and human-readable names
The use of
setUp/tearDownwithreset_translator()ensures proper test isolation. As per coding guidelines, this appears to meet the >80% coverage target for the i18n module.cortex/i18n/locales/fr.yaml (1)
1-307: French translations are complete and properly localized.The translation catalog maintains consistent structure with the English source and correctly preserves all functional
{variable}placeholders for interpolation. Verification confirms all 22 placeholders used in translations ({action},{code},{count},{current},{description},{end},{error},{id},{language},{name},{original},{package},{passed},{path},{provider},{rate},{seconds},{start},{step},{suggested},{total},{version}) match exactly between en.yaml and fr.yaml. Cultural adaptations like "NPD" for "DND" are appropriate.cortex/i18n/config.py (2)
109-128: LGTM!The
set_languagemethod properly validates input, normalizes to lowercase, and provides a clear error message with supported options.
139-176: LGTM!The
get_language_infomethod provides excellent diagnostic information with clear source attribution (environment/config/auto-detected/default), which is valuable for debugging language resolution issues.cortex/cli.py (6)
25-31: LGTM!The i18n imports are well-organized and include all necessary components for CLI internationalization.
158-161: LGTM!Good use of translation keys for user-facing API key messages.
2307-2345: LGTM!Excellent helper function that provides a user-friendly way to specify languages by code, English name, or native name. The case-insensitive matching for Latin scripts while preserving exact matching for non-Latin scripts (like 中文) is thoughtfully implemented.
2348-2386: LGTM!The
_handle_set_languagefunction provides excellent UX by listing all supported languages when an invalid input is provided, helping users discover available options.
2917-2927: LGTM!Smart handling that allows combining language setting with command execution (e.g.,
cortex --set-language es install nginx), providing a convenient one-liner workflow.
2636-2655: LGTM!The config parser setup properly mirrors the implementation with appropriate argument definitions for the language subcommand.
cortex/i18n/formatter.py (3)
14-145: LGTM!Well-researched locale configurations. The French file size units ("o", "Ko", "Mo", "Go", "To") correctly follow the French convention of using "octet" instead of "byte".
284-328: LGTM!The
format_time_agomethod correctly handles singular/plural forms and covers all common time intervals from seconds to years. The early return for negative differences (future dates) gracefully handles edge cases.
169-175: Bug: Language setter checks wrong variable.Line 172 checks
self._language(the current language) instead ofvalue(the new language being set). This means the setter always succeeds if the current language is valid, regardless of whether the new value is valid.🐛 Proposed fix
`@language.setter` def language(self, value: str) -> None: """Set the language.""" - if self._language in LOCALE_CONFIGS: + if value in LOCALE_CONFIGS: self._language = value else: self._language = "en"Likely an incorrect or invalid review comment.
cortex/i18n/translator.py (4)
95-116: LGTM!Good defensive implementation. Using
yaml.safe_loadprotects against arbitrary code execution, and the silent fallback for missing catalogs ensures graceful degradation.
247-262: LGTM!Clean lazy initialization with appropriate use of
LanguageConfigfor language resolution. The debug mode viaCORTEX_I18N_DEBUGenvironment variable is valuable for translation development and testing.
289-309: LGTM!The
t()function provides a clean, concise API for translations throughout the codebase. The docstring examples clearly demonstrate usage patterns.
337-344: LGTM!Returning a copy of
SUPPORTED_LANGUAGESis good defensive programming, preventing accidental mutation of the module-level constant.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai Review. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
@RIVALHIDE Added some comments.
Also, No test for malformed YAML handling. There are still some coderabbit comments left to address.
…f truth for languages, named time constants, non-Latin script handling, and malformed YAML resilience
CLA Verification PassedAll contributors have signed the CLA.
|
|
@RIVALHIDE will it be able to handle translation for any new command that is added to the Cortex in the future ? |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 1503-1530: The output for `config language` leaks untranslated
English: replace raw source keys and hardcoded hint strings in the `--info`
printing block and the "no code" branch with the existing translation calls used
by `_config_show` and other `config.*` keys; specifically, use the `t(...)`
function for labels like environment/source/detected_language (instead of raw
"environment") and swap the hardcoded hints shown in the no-code branch (the two
console.print lines that start with "[dim]Use: cortex config language...") to
call the appropriate `t('config.*')` keys (or reuse the same keys `_config_show`
uses) and output via `cx_print`/`console.print` so all user-facing text in
`get_language`, `SUPPORTED_LANGUAGES` display, and the `--info` branch is
localized.
- Around line 2972-3043: The lookup fails for accented Latin names because
_normalize_for_lookup and _resolve_language_name use .lower(); change
normalization to use .casefold() for ASCII/Latin handling so accented inputs
like "español" or "français" match; update _normalize_for_lookup to return
s.casefold() when _is_ascii(s) (or otherwise when ASCII/Latin detection applies)
and then in _resolve_language_name build keys with english_name.casefold(), use
native_normalized = _normalize_for_lookup(native_name) and still preserve the
original native_name in name_to_code for exact matches; ensure
SUPPORTED_LANGUAGES checks use the same normalization logic.
In `@cortex/i18n/detector.py`:
- Around line 95-121: The locale parsing in the detector (working on
locale_string and LANGUAGE_MAPPINGS) only handles underscores and ignores
hyphenated locales like "en-US"; update the normalization so that hyphens are
converted to underscores (e.g., replace "-" with "_") immediately after
lowercasing/stripping and before removing encoding suffix and performing
lookups, then use the normalized full_locale and language part (from split("_"))
when checking LANGUAGE_MAPPINGS in the existing branches (direct mapping,
language part, regional variant).
In `@cortex/i18n/locales/en.yaml`:
- Around line 120-128: The file contains duplicate top-level keys "cache" and
"env" so later blocks overwrite earlier translations and drop keys like
cache.saved_calls and cache.read_error and many env.* labels; locate all "cache"
and "env" blocks (e.g., the cache block with keys stats_header, hits, misses,
hit_rate, saved_calls, read_error, unexpected_error and the various env.*
labels) and merge their entries into a single "cache" map and a single "env" map
respectively, removing the duplicate blocks and ensuring no keys are lost
(combine values, preferring the most complete/accurate strings for any
conflicting keys). Ensure the merged sections include all previously defined
keys such as cache.saved_calls, cache.read_error, unexpected_error, and all
env.* labels so YAML retains every translation.
In `@cortex/i18n/locales/zh.yaml`:
- Around line 32-44: The YAML defines the top-level mapping key "language" twice
which causes the first block (keys like changed, current, auto_detected,
set_from_env, set_from_config, default, available, invalid_code,
supported_codes, select_prompt, switched_to) to be silently overwritten; merge
the missing key(s) such as set_failed into the first "language" mapping and
remove the duplicate second "language" block so all translations live under a
single "language" mapping (ensure keys like changed, switched_to and set_failed
are present and deduplicated).
In `@tests/test_i18n.py`:
- Around line 854-868: The test method test_cli_language_list contains an unused
import "sys"; remove that import to clean up the test. Edit the
test_cli_language_list function and delete the line importing sys so only the
required imports (patch, argparse, CortexCLI) remain, leaving the rest of the
test (creating argparse.Namespace and calling CortexCLI().config) unchanged.
🧹 Nitpick comments (2)
cortex/i18n/config.py (1)
247-283: Add audit logging for language preference changes.Language updates are persisted but not recorded in the history DB. If audit logging is required for all operations, please record set/clear events (e.g., via
InstallationHistorywith a CONFIG-like type). As per coding guidelines, please log operations to~/.cortex/history.db.tests/test_i18n.py (1)
759-769: Consider usingimportlib.resourcesfor more robust path resolution.The current path construction assumes a specific directory structure relative to the test file. While this works for standard test execution, it could break if the project structure changes.
♻️ Optional: Use importlib.resources for path resolution
def test_all_supported_languages_have_catalogs(self): """Test that all supported languages have message catalogs.""" - from pathlib import Path - + import importlib.resources from cortex.i18n.translator import SUPPORTED_LANGUAGES - locales_dir = Path(__file__).parent.parent / "cortex" / "i18n" / "locales" - for lang_code in SUPPORTED_LANGUAGES: - catalog_path = locales_dir / f"{lang_code}.yaml" - self.assertTrue(catalog_path.exists(), f"Missing catalog for language: {lang_code}") + # Check if resource is accessible within the package + try: + with importlib.resources.files("cortex.i18n.locales").joinpath(f"{lang_code}.yaml").open(): + pass + except FileNotFoundError: + self.fail(f"Missing catalog for language: {lang_code}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/cli.pycortex/i18n/config.pycortex/i18n/detector.pycortex/i18n/formatter.pycortex/i18n/locales/en.yamlcortex/i18n/locales/zh.yamltests/test_i18n.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/i18n/formatter.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pytests/test_i18n.pycortex/i18n/config.pycortex/i18n/detector.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_i18n.py
🧬 Code graph analysis (2)
tests/test_i18n.py (4)
cortex/i18n/formatter.py (13)
language(181-183)language(186-191)LocaleFormatter(164-367)format_date(197-208)format_date(400-402)format_number(238-265)format_number(415-417)format_file_size(267-298)format_file_size(420-422)format_datetime(223-236)format_datetime(410-412)format_time(210-221)format_time(405-407)cortex/i18n/config.py (3)
set_language(247-273)get_language(211-245)LanguageConfig(50-328)cortex/cli.py (5)
config(1469-1483)CortexCLI(62-2969)_resolve_language_name(2997-3053)_handle_set_language(3056-3094)main(3158-3997)cortex/i18n/detector.py (2)
detect_os_language(124-164)get_os_locale_info(167-182)
cortex/i18n/config.py (2)
cortex/i18n/detector.py (1)
detect_os_language(124-164)cortex/i18n/translator.py (5)
t(291-311)get_language(281-288)set_language(267-278)language(61-63)language(66-83)
🪛 YAMLlint (1.37.1)
cortex/i18n/locales/en.yaml
[error] 431-431: duplication of key "env" in mapping
(key-duplicates)
[error] 459-459: duplication of key "cache" in mapping
(key-duplicates)
cortex/i18n/locales/zh.yaml
[error] 131-131: duplication of key "language" in mapping
(key-duplicates)
🔇 Additional comments (9)
tests/test_i18n.py (9)
1-19: LGTM!The module docstring clearly documents the test coverage areas, and the imports are appropriate for the test suite.
21-212: LGTM!The
TestTranslatorclass provides comprehensive coverage of the translator functionality including language switching, variable interpolation, debug mode, singleton behavior, and translations across all 5 supported languages. The setUp/tearDown methods properly reset the global translator state for test isolation.
235-346: LGTM!Excellent coverage of malformed YAML edge cases. These tests validate the production hardening mentioned in the commit messages, ensuring the config loader is resilient to:
- Malformed YAML syntax
- Empty files
- Whitespace-only files
- Invalid root types (list, string instead of dict)
- Non-string language values
- Null language values
This defensive testing prevents crashes in production when users have corrupted config files.
423-528: LGTM!The environment variable precedence tests correctly verify the documented priority order:
LANGUAGE→LC_ALL→LC_MESSAGES→LANG. The fallback scenarios (unsupported languages, C/POSIX locales, empty environment) are well covered.
530-754: LGTM!The
TestLocaleFormatterclass provides solid coverage of locale-aware formatting including dates, numbers, file sizes, relative times, and durations across multiple locales. The invalid language fallback test (lines 748-753) ensures robustness.
788-832: LGTM!The
TestTranslationCompletenessclass directly addresses the PR objective of ensuring "complete message translation with graceful fallback to English (avoid English leaks)". The tests verify that:
- Common keys are translated in all languages
- Spanish translations aren't just English placeholders
- Variable interpolation works correctly across all languages
964-1037: LGTM!The
TestResolveLanguageNameclass provides excellent coverage of the language resolution logic, including:
- Direct code matches
- English name resolution (case-insensitive)
- Native name resolution (including non-Latin scripts like 中文)
- Whitespace handling
- Invalid/unsupported language rejection
The tests for non-Latin script handling (lines 1013-1029) directly validate the PR's handling of scripts where
.lower()is meaningless.
1039-1102: LGTM!The
TestSetLanguageFlagclass validates the--set-languageglobal CLI flag, ensuring it correctly handles language codes, English names, native names, and invalid input with appropriate exit codes.
1-1105: Multi-process file locking is already properly implemented incortex/i18n/config.py.The
LanguageConfigclass uses boththreading.Lockfor intra-process synchronization (line 73) andfcntl.flockfor inter-process synchronization (lines 96-118), with platform-specific handling for Windows. The implementation includes exclusive locks for writes and shared locks for reads, with comprehensive error handling and documentation. No additional concurrency tests are needed intest_i18n.pyas the codebase has a dedicatedtest_thread_safety.pyfor concurrent access scenarios.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| cache: | ||
| stats_header: "Cache Stats" | ||
| hits: "Hits" | ||
| misses: "Misses" | ||
| hit_rate: "Hit rate" | ||
| saved_calls: "Saved calls (approx)" | ||
| read_error: "Unable to read cache stats: {error}" | ||
| unexpected_error: "Unexpected error reading cache stats: {error}" | ||
|
|
There was a problem hiding this comment.
Consolidate duplicate env and cache sections to avoid missing translations.
YAML keeps the last occurrence of a key, so the earlier env and cache blocks are overwritten. This drops keys such as cache.saved_calls, cache.read_error, and many extended env.* labels. Merge into single sections.
🔧 Proposed fix (merge cache, remove duplicate env)
cache:
stats_header: "Cache Stats"
hits: "Hits"
misses: "Misses"
hit_rate: "Hit rate"
saved_calls: "Saved calls (approx)"
+ entries: "Total Entries"
+ size: "Cache Size"
+ cleared: "Cache cleared"
read_error: "Unable to read cache stats: {error}"
unexpected_error: "Unexpected error reading cache stats: {error}"
@@
-# =============================================================================
-# Environment management
-# =============================================================================
-env:
- audit_header: "Environment Audit"
- config_files_scanned: "Config Files Scanned"
- variables_found: "Environment Variables Found"
-
- # PATH operations
- path_added: "Added '{path}' to PATH"
- path_removed: "Removed '{path}' from PATH"
- path_already_exists: "'{path}' is already in PATH"
- path_not_found: "'{path}' not found in PATH"
-
- # {path} - config file path
- persist_note: "To use in current shell: source {path}"
-
-# =============================================================================
-# Cache
-# =============================================================================
-cache:
- stats_header: "LLM Cache Statistics"
- hits: "Cache Hits"
- misses: "Cache Misses"
- hit_rate: "Hit Rate: {rate}%"
- entries: "Total Entries"
- size: "Cache Size"
- cleared: "Cache cleared"Also applies to: 376-406, 431-444, 459-468
🤖 Prompt for AI Agents
In `@cortex/i18n/locales/en.yaml` around lines 120 - 128, The file contains
duplicate top-level keys "cache" and "env" so later blocks overwrite earlier
translations and drop keys like cache.saved_calls and cache.read_error and many
env.* labels; locate all "cache" and "env" blocks (e.g., the cache block with
keys stats_header, hits, misses, hit_rate, saved_calls, read_error,
unexpected_error and the various env.* labels) and merge their entries into a
single "cache" map and a single "env" map respectively, removing the duplicate
blocks and ensuring no keys are lost (combine values, preferring the most
complete/accurate strings for any conflicting keys). Ensure the merged sections
include all previously defined keys such as cache.saved_calls, cache.read_error,
unexpected_error, and all env.* labels so YAML retains every translation.
|
@Suyashd999 Yes, the system supports new commands, but developers must manually add translation keys to en.yaml and other locale files — there's no auto-translation. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@RIVALHIDE Kindly address bot comments and then create a new Issue regarding auto-translation thing.
|
3 participants
Related Issue
Closes #93
Summary
Adds comprehensive multi-language support (i18n) to the Cortex CLI with proper localization, formatting, tests, and documentation.
Supported Languages
enesfrdezhKey Features
cortex config language <code>orcortex --set-language "English"LANG,LC_ALL,LANGUAGEenvironment variables~/.cortex/preferences.yamlCORTEX_I18N_DEBUG=1) to show translation keysUsage Examples
Set language by code
cortex config language es
Set language by name (human-readable)
cortex --set-language "Spanish"
View current language info
cortex config language --info
List available languages
cortex config language --list
Use auto-detection
cortex config language --auto
Clear preference (use OS default)
cortex config language --clear
Files Changed
pyproject.tomlcortex.i18npackage, locale YAML files as package data, upgraded setuptools to >=64.0 for editable install supportMANIFEST.inrecursive-include cortex/i18n/locales *.yamlto include translations in PyPI distributioncortex/cli.pyt()calls, addedconfig languagesubcommand, added--set-languageglobal flagcortex/i18n/__init__.pyt,set_language,get_language, etc.)cortex/i18n/translator.pycortex/i18n/config.py~/.cortex/preferences.yaml, preference resolution ordercortex/i18n/detector.pyLANG,LC_ALL,LANGUAGE,LC_MESSAGES)cortex/i18n/formatter.pycortex/i18n/locales/en.yamlcortex/i18n/locales/es.yamlcortex/i18n/locales/fr.yamlcortex/i18n/locales/de.yamlcortex/i18n/locales/zh.yamltests/test_i18n.pydocs/i18n-guide.mdAI Disclosure
Used Cursor AI (Claude opus 4.5) to assist with:
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Architecture
Language Resolution Order
CORTEX_LANGUAGEenvironment variable (highest priority)~/.cortex/preferences.yamlLANG/LC_ALLDemo video
Screencast.from.14-01-26.07.13.17.PM.IST.webm
Summary by CodeRabbit
New Features
cortex config language(list/info/set),cortex config show, and global--language/--set-languagehandling.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.