Skip to content

fix: address i18n review issues from PR #718#758

Merged
igorls merged 1 commit intoMemPalace:developfrom
mvalentsev:fix/i18n-review-issues
Apr 15, 2026
Merged

fix: address i18n review issues from PR #718#758
igorls merged 1 commit intoMemPalace:developfrom
mvalentsev:fix/i18n-review-issues

Conversation

@mvalentsev
Copy link
Copy Markdown
Contributor

Addresses three issues bensig flagged on PR #718 before merge.

1. ko.json variable name mismatch (line 28)
status_drawers used {drawers} while all other 7 languages use {count}. The t() call passes count=N, so Korean showed the raw template string instead of the number. One-character fix.

2. Test file shipped inside the installed package
mempalace/i18n/test_i18n.py used a sys.path.insert hack to work around its wrong location. Moved to tests/test_i18n.py per AGENTS.md convention. Removed the sys.path hack and the __name__ runner block.

3. Dialect.from_config() state leak
from_config passed lang=config.get("lang") which defaults to None. When no "lang" key is in the config, __init__ fell through to current_lang() -- a module-level global set by whichever load_lang() call ran most recently. Now defaults to "en" explicitly so from_config is deterministic.

Bug 1 from bensig's review (compress() never uses self.aaak_instruction or self.lang_regex) is intentionally not addressed here -- bensig suggested deferring the regex wiring until alignment with #488/#507 on extraction approach.

Changes (3 files, +19/-17):

  • mempalace/i18n/ko.json -- {drawers} -> {count}
  • mempalace/dialect.py -- config.get("lang") -> config.get("lang", "en")
  • tests/test_i18n.py -- moved from mempalace/i18n/, added 2 regression tests

References: bensig's review comments on PR #718 (2026-04-12T21:54Z and 22:01Z)

@mvalentsev mvalentsev marked this pull request as ready for review April 13, 2026 05:22
@mvalentsev mvalentsev force-pushed the fix/i18n-review-issues branch from 0df7313 to 4b7c551 Compare April 13, 2026 21:46
@igorls igorls added the area/i18n Multilingual, Unicode, non-English embeddings label Apr 14, 2026
@mvalentsev mvalentsev force-pushed the fix/i18n-review-issues branch from 4b7c551 to f40aac4 Compare April 14, 2026 03:32
Three issues flagged by bensig on the i18n PR before merge:

1. ko.json: status_drawers used {drawers} instead of {count}, causing
   the Korean UI to show the raw template string instead of the actual
   drawer count.  All other 7 languages use {count}.

2. Test file was shipped inside the package at mempalace/i18n/test_i18n.py
   with a sys.path.insert hack.  Moved to tests/test_i18n.py per the
   project convention in AGENTS.md.

3. Dialect.from_config() passed lang=config.get("lang") which defaults
   to None, causing __init__ to inherit whatever language was loaded
   earlier via module-level state.  Now defaults to "en" explicitly so
   from_config is deterministic regardless of prior load_lang() calls.

Added two regression tests for the ko.json fix and the state leak.
@mvalentsev mvalentsev force-pushed the fix/i18n-review-issues branch from f40aac4 to d565718 Compare April 15, 2026 06:03
@igorls igorls merged commit 312b3b5 into MemPalace:develop Apr 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/i18n Multilingual, Unicode, non-English embeddings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants