Replace UI text with Chinese (zh) translation#5671
Conversation
已翻译文件： - 404.htm, dmxmap.htm, edit.htm, index.htm - liveview.htm, liveviewws2D.htm, msg.htm - settings.htm, settings_2D.htm, settings_dmx.htm - settings_leds.htm, settings_pin.htm, settings_pininfo.htm - settings_sec.htm, settings_sync.htm, settings_time.htm - settings_ui.htm, settings_um.htm, settings_wifi.htm - update.htm, usermod.htm, welcome.htm 翻译内容：标题、按钮、标签、提示文字、错误信息 保持不变：HTML结构、JS代码、CSS、变量名、URL
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAll web UI files under wled00/data/ were localized from English to Chinese: each page’s root ChangesComplete Web UI Chinese Localization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/data/settings_leds.htm (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate document language metadata to Chinese.
Line 2 is still
lang="en"while the page content is localized to Chinese; switch tolang="zh"to keep accessibility and language detection correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/settings_leds.htm` at line 2, The page's HTML root element still declares lang="en" while the content is Chinese; update the <html> element in settings_leds.htm from lang="en" to lang="zh" (or a more specific variant like lang="zh-CN" if appropriate) so language metadata matches the localized content and aids accessibility and language detection.wled00/data/update.htm (1)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize auto-update “firmware not found” status
When no matching asset is found, the status message is still English (
Firmware not found... Available...). This is user-visible in the Chinese workflow and should be translated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/update.htm` around lines 99 - 101, The status message shown when no matching asset is found uses hardcoded English in the assignment to status.textContent (currently using `Firmware not found (*${assetSuffix}). Available: ${available}`); replace that literal with the localized string lookup used elsewhere (e.g., call the project's i18n function or lookup table) and format it with assetSuffix and available, so the message is translated for other locales; ensure you still set btn.disabled = false and keep the `available` and `assetSuffix` variables in the formatted localized message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/data/settings_dmx.htm`:
- Line 58: The label text contains an English word "Universe" breaking
localization; update the string in the settings_dmx.htm line that renders "代理
Universe <input name=PU ..." (input name=PU) to a fully localized Chinese term
(e.g., "代理 宇宙 <input name=PU ...> 从 E1.31 到 DMX（0=禁用）") so the UI is consistent.
In `@wled00/data/settings_sec.htm`:
- Line 50: The tooltip for the PIN input is in English; update the input element
with id="PIN" (the PIN <input> element) to use a Chinese title string instead of
"Please enter a 4 digit number" so it matches the localized page (e.g. "请输入4位数字"
or similar), keeping the other attributes (type, id, name, size, maxlength,
minlength, onkeydown, pattern, inputmode) unchanged.
- Line 77: Replace the literal English "Upload" button labels with the Chinese
localized text by changing the button text nodes for the buttons that call
uploadFile (the <button ... onclick="uploadFile(...)"> elements) to "上传" (or the
project's Chinese localization token if one exists); update both occurrences
noted (the buttons near the file input for presets and the other at line 79) and
keep the onclick/uploadFile calls and attributes unchanged so functionality is
preserved.
In `@wled00/data/settings_sync.htm`:
- Line 218: The label text containing the English word "presets" should be
localized; locate the HTML snippet containing the input element with name="AP"
and replace the trailing English word "presets" with the Chinese localized term
(e.g., "预设") so the line reads consistently in Chinese and preserves the
existing input markup.
In `@wled00/data/settings_wifi.htm`:
- Line 263: The AP password input element (name="AP", pattern="(.{8,63})|()")
currently uses an English title "Empty or min. 8 characters"; update that title
to the Chinese translation so the validation hint is localized (e.g.
"为空或至少8个字符") while leaving the pattern and maxlength unchanged.
- Around line 126-130: The encryption option labels in the template string
assigned to encryptionTypeField are still in English; update the displayed
option text for option elements (the ones rendered for id/name "ET${i}") to use
localized strings or bilingual wording consistent with the page (e.g., replace
"None/WPA/WPA2" and "WPA/WPA2-Enterprise" with localized equivalents or add the
translated text alongside the English text). Ensure the change updates the
template that builds the select for ET${i} so the selected logic ((type==0) /
(type==1)) remains intact and onchange="E(${i})" continues to work.
In `@wled00/data/update.htm`:
- Around line 136-139: The placeholders "Loading..." and "Checking..." are still
in English; update the HTML in update.htm to use Chinese localized text (e.g.,
"加载中..." for class="sip installed-version" and class="sip release-name", and
"检查中..." for the span with id="Norelease-download") so the UI is consistent;
adjust the content for the span id="release-download" if it contains any English
placeholder text as well and ensure all three referenced elements ("sip
installed-version", "sip release-name", "Norelease-download", and
"release-download") display the Chinese strings.
---
Outside diff comments:
In `@wled00/data/settings_leds.htm`:
- Line 2: The page's HTML root element still declares lang="en" while the
content is Chinese; update the <html> element in settings_leds.htm from
lang="en" to lang="zh" (or a more specific variant like lang="zh-CN" if
appropriate) so language metadata matches the localized content and aids
accessibility and language detection.
In `@wled00/data/update.htm`:
- Around line 99-101: The status message shown when no matching asset is found
uses hardcoded English in the assignment to status.textContent (currently using
`Firmware not found (*${assetSuffix}). Available: ${available}`); replace that
literal with the localized string lookup used elsewhere (e.g., call the
project's i18n function or lookup table) and format it with assetSuffix and
available, so the message is translated for other locales; ensure you still set
btn.disabled = false and keep the `available` and `assetSuffix` variables in the
formatted localized message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 065507ad-8cc4-4391-974f-e6041ae4e794
📒 Files selected for processing (22)
wled00/data/404.htmwled00/data/dmxmap.htmwled00/data/edit.htmwled00/data/index.htmwled00/data/liveview.htmwled00/data/liveviewws2D.htmwled00/data/msg.htmwled00/data/settings.htmwled00/data/settings_2D.htmwled00/data/settings_dmx.htmwled00/data/settings_leds.htmwled00/data/settings_pin.htmwled00/data/settings_pininfo.htmwled00/data/settings_sec.htmwled00/data/settings_sync.htmwled00/data/settings_time.htmwled00/data/settings_ui.htmwled00/data/settings_um.htmwled00/data/settings_wifi.htmwled00/data/update.htmwled00/data/usermod.htmwled00/data/welcome.htm
| Proxy Universe <input name=PU type=number min=0 max=63999 required> from E1.31 to DMX (0=disabled)<br> | ||
| <i>This will disable the LED data output to DMX configurable below</i><br><br> | ||
| <i>Number of fixtures is taken from LED config page</i><br> | ||
| 代理 Universe <input name=PU type=number min=0 max=63999 required> 从 E1.31 到 DMX（0=禁用）<br> |
There was a problem hiding this comment.
Translate mixed-language label for consistency
Line 58 still contains Universe in English (代理 Universe ...), which breaks the otherwise fully localized UI on this page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/data/settings_dmx.htm` at line 58, The label text contains an English
word "Universe" breaking localization; update the string in the settings_dmx.htm
line that renders "代理 Universe <input name=PU ..." (input name=PU) to a fully
localized Chinese term (e.g., "代理 宇宙 <input name=PU ...> 从 E1.31 到 DMX（0=禁用）")
so the UI is consistent.
- settings_leds.htm: lang=en → lang=zh - settings_dmx.htm: Universe 补冒号 - settings_sec.htm: PIN 提示、Upload 按钮翻译 - settings_sync.htm: presets → 个预设 - settings_wifi.htm: AP 密码提示、加密类型翻译 - update.htm: Loading/Checking/Firmware not found 翻译
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
thanks, but it is not "add" but "replace with". There is currently no way for multi language support, this has been attempted several times already for other languages but simply replacing everything is not acceptable. You can keep this in your fork though. |
DedeHai
changed the title
|
Hi @DedeHai, thanks for the feedback. I understand that directly replacing all English text is not acceptable for upstream. I have a question about the multi-language approach: would a build-time locale approach be viable? The idea is:
This avoids the runtime switching problem entirely. The trade-off is that users need to pick their language at build time, but for non-English users this is usually a one-time choice. Is this something the project would accept? If so, I can rework this PR along those lines. If not, could you share what approach (if any) would be acceptable for getting non-English support into upstream? Thanks! |
|
In theory yes but it eludes me on how you want to do that. Like replace the htm files with different versions at compile time? how to maintain that? |
|
Good question. Here is the concrete approach I have in mind: Implementation: PlatformIO pre-build script
Maintenance:
Flash impact:
What the user gets:
Would this approach work for you? If so, I can rework this PR to implement it. The current 22 translated HTML files would be converted into one |
|
@netmindz what do you think of the approach proposed? |
|
You are right to question the approach. After thinking more carefully, here is a better design that avoids touching the original HTML entirely: Core idea: External toolchain using an HTML parser (BeautifulSoup), not regex. The problem with regex-based string replacement is that it is fragile — any HTML structure change breaks it. Instead, we use BeautifulSoup to parse the DOM tree, extract translatable text with its DOM path, and replace precisely by node location. How it works:
Why this is better than regex:
Zero impact on WLED codebase:
WLED contributors do not need to care about translations at all. They edit English HTML as always. The toolchain handles everything externally. Would you accept a PR that adds this toolchain to |
|
@foxlesbiao please use English commit descriptions. Repository language is English, and personally i cannot understand Japanese. |
@foxlesbiao we have a lot of dynamic changes in the WebUI, usually by JS code modifying the DOM on-the-fly. Would your idea still work with this? examples: WLED/wled00/data/settings_leds.htm Line 154 in 3a50268 WLED/wled00/data/settings_leds.htm Lines 311 to 315 in 3a50268 WLED/wled00/data/settings_leds.htm Line 501 in 3a50268 Line 928 in 5f79107 this function shows another UI pattern used in WLED - the .htm only contains a skeleton, and the real content (in this case: list of effects) is populated by reading out a JSON endpoint provided by WLED. |
|
@coderabbitai the WLED webUI has a lot of dynamically changing content, both by using embedded JS and by dynamically generated content like |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# 1. Check the specific lines softhack007 mentioned in settings_leds.htm
echo "=== settings_leds.htm line 140-165 (around line 154) ==="
sed -n '140,165p' wled00/data/settings_leds.htm
echo ""
echo "=== settings_leds.htm line 300-325 (around lines 311-315) ==="
sed -n '300,325p' wled00/data/settings_leds.htm
echo ""
echo "=== settings_leds.htm line 490-515 (around line 501) ==="
sed -n '490,515p' wled00/data/settings_leds.htm
echo ""
echo "=== All settingsScript.print() usage in translated files ==="
rg -n 'settingsScript\.print' wled00/data/ --include='*.htm'
echo ""
echo "=== innerHTML assignments inside <script> blocks across translated files ==="
rg -n 'innerHTML\s*[+=]' wled00/data/ --include='*.htm' -C2
echo ""
echo "=== document.write() calls in translated files ==="
rg -n 'document\.write' wled00/data/ --include='*.htm'
echo ""
echo "=== Check for JS syntax issues: Chinese chars inside JS string literals ==="
# Look for lines where Chinese characters appear inside what looks like a JS string assignment
rg -n '(?:=\s*["\`]|["\`]\s*\+)[^"]*[\x{4e00}-\x{9fff}]' wled00/data/ --include='*.htm' -C1
echo ""
echo "=== Check for broken JS template literals with Chinese chars ==="
rg -n '\$\{[^}]*[\x{4e00}-\x{9fff}][^}]*\}' wled00/data/ --include='*.htm' -C2
echo ""
echo "=== settings_leds.htm: all innerHTML assignments with Chinese text ==="
grep -n 'innerHTML' wled00/data/settings_leds.htm | head -60
echo ""
echo "=== settings_wifi.htm: all innerHTML / JS-generated content ==="
grep -n 'innerHTML\|document\.write\|\.print\|settingsScript' wled00/data/settings_wifi.htm
echo ""
echo "=== settings_time.htm: JS-generated strings (timer/preset labels) ==="
grep -n 'innerHTML\|document\.write\|\.print' wled00/data/settings_time.htm | head -40
echo ""
echo "=== Check settings_leds.htm for mixed English/Chinese content (missed translations) ==="
# Lines that contain English words that look like UI labels (not JS variable names)
grep -n '"[A-Z][a-z]' wled00/data/settings_leds.htm | grep -v '//\|url\|http\|var \|function\|const \|let \|return\|console\|class\|id=\|name=\|type=\|value=\|data-' | head -40
echo ""
echo "=== Full settings_leds.htm line count ==="
wc -l wled00/data/settings_leds.htm
echo ""
echo "=== settings_2D.htm: JS-generated panel label content ==="
grep -n 'innerHTML\|addPanel\|\.print\|settingsScript' wled00/data/settings_2D.htm | head -30
echo ""
echo "=== index.htm: dynamic content patterns ==="
grep -n 'innerHTML\|document\.write\|settingsScript' wled00/data/index.htm | head -40Length of output: 10902
1. Dynamic UI behaviour — is it broken?No, the existing JS logic itself is not broken. The translated files preserve all JavaScript functions, DOM manipulation calls, event handlers, and 2. Incomplete translation — mixed-language alert/error messagesSeveral JS
By contrast, the JS-generated 3. Critical limitation of the proposed BeautifulSoup toolchainThis is the more fundamental issue you were hinting at:
No — and this is a significant structural problem with the proposed approach:
Summary
For the toolchain to be complete it would need a JS parser (e.g. 🧠 Learnings used |
Build-time translation system with zero runtime/flash overhead. Tools: - tools/i18n/extract.py: Extract translatable strings from HTM files - tools/i18n/build.py: Apply translations at PlatformIO pre-build - tools/i18n/locales/zh_CN.json: Chinese (Simplified) translation (324/472 strings) Coverage: 472 strings across 22 HTM files (HTML text + JS strings + attributes). PlatformIO integration via -D WLED_LOCALE=zh_CN build flag. See tools/i18n/README.md for details.
Fill 148 missing translations (coverage 68.6% -> 98.9%). All alert(), innerHTML, innerText strings in settings_leds.htm identified by softhack007 are now translated: - L146: Invalid Bus-config -> 无效的总线配置 - L154: HUB75 error: panels≠rows×cols -> HUB75 错误：面板数 ≠ 行×列 - L311: No. of Panels -> 面板数量 - L314: rows x cols -> 行 x 列 - L501: 80% of max LED memory -> 80% 的最大 LED 内存 Remaining 1% (5 entries): HTML fragments and template literals that cannot be translated at build time.
|
Hi all, thanks for the thorough feedback. I've reworked the approach completely based on your input. New approach: Build-time i18n toolchainInstead of replacing HTML files directly, I've built an external toolchain that lives in How it worksZero changes to existing WLED codebase. The i18n toolchain is fully external — English HTML remains the single source of truth, contributors edit English as always. Addressing @softhack007's concerns
Good question. I analyzed the three categories:
Specifically for the examples you raised:
The Addressing @coderabbitai's analysis
This is correct for BeautifulSoup, but the toolchain uses regex pattern extraction for script blocks, not DOM parsing. It matches
True, but as analyzed above, 99% of Current stats
For other languagesAdding a new language = one JSON file + one build env: python3 tools/i18n/extract.py
cp tools/i18n/locales/_template.json tools/i18n/locales/de_DE.json
# translator fills in "translation" fieldsPlatformIO integration: [env:esp32dev_zh_CN]
extends = env:esp32dev
build_flags = ${env:esp32dev.build_flags} -D WLED_LOCALE=zh_CN
extra_scripts = pre:tools/i18n/build.pyNext stepsIf this approach looks viable, I can:
Would appreciate your thoughts on this direction. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch toolchain files from foxlesbiao's i18n-toolchain branch
gh api repos/foxlesbiao/WLED/git/trees/i18n-toolchain --jq '.tree[] | .path' 2>/dev/null | head -30
echo "=== tools/i18n directory tree ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n?ref=i18n-toolchain" --jq '.[].name' 2>/dev/null
echo "=== extract.py ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/extract.py?ref=i18n-toolchain" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null
echo "=== build.py ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/build.py?ref=i18n-toolchain" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null
echo "=== locales directory ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/locales?ref=i18n-toolchain" --jq '.[].name' 2>/dev/null
echo "=== First 100 lines of zh_CN.json to see key format ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/locales/zh_CN.json?ref=i18n-toolchain" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | head -100Length of output: 22893 Thanks for implementing this and sharing the actual code, 🔴 Critical issues in
|
| Issue | Severity | Impact |
|---|---|---|
| Cross-file HTML replacement (no file guard) | 🔴 Critical | Silent wrong replacements across all files |
replace_html_text matches inside <script> |
🔴 Critical | JS corruption |
replace_js_string spans script block boundaries |
🔴 Critical | Wrong replacement / missed replacement |
| Default output overwrites source files | 🔴 Critical | Destroys English source on accidental run |
Template literals with ${} untranslatable |
🟡 Known gap | ~5–10 strings missed |
value attribute over-extraction |
🟡 Minor | Extra noise in template JSON |
The core architecture (DOM path extraction + JSON locale files + PlatformIO hook) is sound. The issues above are all fixable — but they need to be resolved before the toolchain can be trusted to produce correct output, especially since issues #1 and #2 can silently introduce broken JavaScript in the generated files.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-05-25T20:45:31.263Z
Learning: In the WLED project (`wled/WLED`), `// AI: below section was generated by an AI` comments in webUI files (e.g., `wled00/data/settings_wifi.htm`) are an intentional project convention to highlight AI-generated code blocks. These comments are stripped out during the uglify/minify firmware build process and should not be flagged for removal. Only the *quality and accuracy of documentation within those blocks* is subject to review.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-05-23T22:42:03.102Z
Learning: In the WLED repo (wled/WLED), .coderabbit.yaml path_instructions use inline instruction strings only — there is no file: reference syntax to point to external instruction files. The trust boundary model for security reviews is: firmware C++ files under wled00/ and usermods/ get security rules applied only at untrusted ingress points (HTTP/JSON API, WebSocket, UDP datagrams via parsePacket()/recvfrom(), TCP reads, serial/UART). Files already validated at ingress are trusted downstream. WebUI files under wled00/data/ are always treated as an ingress/output surface and the trust boundary reduction does NOT apply there.
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
1. File-scoped HTML replacement: translations only apply to their source file, preventing cross-file bleed (e.g. welcome.htm's 'WiFi settings' no longer affects index.htm) 2. Script-block-aware processing: split content into script/non-script segments, apply HTML replacements only to non-script segments, preventing JS corruption 3. Per-script-block JS matching: replace_js_in_block() operates on individual <script>...</script> blocks instead of the entire file, preventing cross-block matching errors 4. Safe default output: --output-dir defaults to a temp directory instead of overwriting English source files. Added safety warning when output == source.
|
Thanks for the thorough code review, Fixes appliedBug #1 — Cross-file HTML replacement: Bug #2 — Bug #3 — Bug #4 — Default output overwrites source: Bug #6 —
|
Most value attributes are form codes (value="0", value="1"), not user-visible text. Button label values are captured as HTML text content instead.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Updated build.py ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/build.py?ref=i18n-toolchain" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null
echo ""
echo "=== Updated extract.py (TRANSLATABLE_ATTRS section) ==="
gh api "repos/foxlesbiao/WLED/contents/tools/i18n/extract.py?ref=i18n-toolchain" --jq '.content' 2>/dev/null | base64 -d 2>/dev/nullLength of output: 20757
🟡 Bug
|
PlatformIO users commonly write -DWLED_LOCALE=zh_CN without a space after -D. Use regex match instead of substring search.
|
Thanks for the follow-up review, Bug #6 (
BUILD_FLAGS parsing: Fixed. Now uses regex All issues resolved. Ready for maintainer review. (ﾐ^ ◡ ^ﾐ) |
|
Just for completeness, there are additional translation challenges that we might not be able to address. A few examples that come to my mind:
There might be more. Languages and internationalization is a complicated field, its not just translation of sentence fragments. |
- fx_zh_CN.json: Complete Chinese translations for all effect names and palette names - Used by build.py --cpp-headers to generate locale_effects.h and locale_palettes.h - 100% coverage for effects and palettes
Update: Layer 3/4 Implementation CompleteHi all, I've implemented Layer 3 (effects) and Layer 4 (palettes) based on the proposal v3 approach: What's NewLayer 3: Effect Names (PROGMEM replacement)
Layer 4: Palette Names (PROGMEM replacement)
Architecture (as discussed)UsageWLED i18n Build — zh_CN404.htm: 3 translations Total: 416 translations applied across 22 files Coverage Summary
Repositories
The toolchain is ready for review. The translation files can be moved to once the repo is created. @softhack007 @DedeHai @netmindz — Ready for your review! |
This comment was marked as resolved.
This comment was marked as resolved.
|
@softhack007 Hey, here's the proposal I put together. Could you take a look and let me know if this approach works? Based on your feedback, I've implemented:
Repo structure: Effect names and palette names are translated via C++ PROGMEM replacement ( Regarding the word order / number format / grammar issues you mentioned — these do exist in theory, but WLED UI mostly uses short labels ("Brightness", "Speed"), not full sentences, so the practical impact is minimal. Looking forward to your feedback! |
- Two-repository architecture (toolchain + translations) - Four-layer translation system (L1-L4) - Build flow with PlatformIO integration - Dynamic content handling via PROGMEM replacement - Grammar/word order analysis - Coverage summary
This comment was marked as resolved.
This comment was marked as resolved.
The problem has been solved |
|
thanks, I like the general approach of extracting strings to json as a template. I assume the tools are fully AI generated correct? speaking of pixelforge: the external tools will always be in english as they are downloaded on-the-fly. I do agree that translations should be out-of-tree and maintained by contributors. Does the build tool support that? One question is also on how to run tests before a release to check if the translation/extraction toolchain works. edit: |
- Revert 22 translated .htm files to original English - Add --validate flag to extract.py for CI/coverage checking - Update README with out-of-tree workflow (similar to usermods) - Translations now maintained in WLED-translations repo Users build translated firmware locally: python3 tools/i18n/extract.py --validate zh_CN python3 tools/i18n/build.py --locale zh_CN --translation-dir ../WLED-translations/zh_CN
Done. Reverted all translated .htm files, PR now only contains tools/i18n/ framework. Translations live in WLED-translations repo. Added --validate flag for CI checks. Build workflow similar to usermods. |
|
downloading the repo manually is not how out of tree usermods are compiled, can it be done in the same way? |
- build.py v4: auto-detect translations in .pio/libdeps/ - Remove locale files from tools/i18n/locales/ (moved to WLED-translations repo) - Update docs with PlatformIO out-of-tree workflow - Users: add custom_usermods = https://github.com/foxlesbiao/WLED-translations - Translators: work in WLED-translations repo, no WLED fork needed
|
|
please revert the htm file changes, they are still there. I tried your script, it does not work at all with the suggested workflow. |
|
Looking at your effect string replacements, for example It seems that all effect metadata (the part after |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tools/i18n/README.md (1)
7-19: ⚡ Quick winAdd fence languages to satisfy markdown linting.
Please label these fenced blocks (e.g.,
text) so MD040 doesn’t fail.Also applies to: 41-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/README.md` around lines 7 - 19, The fenced code blocks in tools/i18n/README.md are missing language labels which triggers MD040; update each triple-backtick block (the ASCII flow diagrams around the extract/build steps and the similar block later) by adding a language identifier such as text (i.e., change ``` to ```text) so the markdown linter recognizes the fence language; ensure you update both occurrences referenced in the comment so all fenced blocks include the label.Source: Linters/SAST tools
tools/i18n/ARCHITECTURE.md (1)
7-23: ⚡ Quick winSpecify a language for fenced code blocks.
markdownlintMD040 is triggered here. Add a language (e.g.,text) to each ASCII/structure block fence.Also applies to: 67-75, 112-120, 124-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/ARCHITECTURE.md` around lines 7 - 23, Update the fenced ASCII diagram blocks so they include a language tag (e.g., use ```text instead of ```) to satisfy markdownlint MD040; specifically edit the fences that wrap the diagrams showing "Core Repo (WLED/tools/i18n/)" and "Translation Repo (WLED-translations/<locale>/)" and the other ASCII/structure blocks referenced (the ones with the box characters and lists like extract.py, build.py, static.json, js.json, effects.json, palettes.json, metadata.json) to use a language identifier such as text for each opening fence.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/i18n/build.py`:
- Around line 96-97: Remove the unnecessary f-string prefixes from the static
print statements so Ruff F541 is not triggered: change the two prints shown
(print(f" Searched:", file=sys.stderr) and print(f" --translations-dir (if
provided)", file=sys.stderr)) to plain string prints (no leading f), and make
the same change for the other occurrences flagged (the analogous prints at the
later section referenced around the second pair). Locate and update the exact
print calls in build.py (the static stderr print statements) to remove the "f"
prefix while leaving the rest of the call unchanged.
- Around line 125-130: load_translations() currently drops entries with empty
entry['translation'], causing validate_translations() to only see translated
keys and report false success; change load_translations() (the block that builds
merged[fname][key]) to always include keys from the source (store translation as
'' or None when missing) so merged retains every original key, and update
validate_translations() to compute completeness against the full set in merged
(checking translation==''/None as missing). Ensure references: function names
load_translations(), validate_translations(), the merged dict, and the entry
{'translation','en'} handling are updated so untranslated keys are preserved and
counted as missing.
- Around line 108-112: The current logic in build.py first sets files_to_load =
sorted(tdir.glob("*.json")) and only replaces it with the single file when
single.exists() and single not in files_to_load, which means when single is
present alongside other *.json files (e.g., in tools/i18n/locales/) other locale
files still get loaded; change the branch so that if the specific locale file
(single) exists you set files_to_load = [single] unconditionally (or filter
files_to_load to only include single) instead of only when it's not already in
the glob list, referencing the variables/files_to_load, tdir, single, and locale
to locate the change.
In `@tools/i18n/extract.py`:
- Line 325: The print call uses an unnecessary f-string (print(f"\nPASSED: All
strings translated")) which triggers Ruff F541; change it to a plain string by
removing the leading f (use print("\nPASSED: All strings translated")) and scan
nearby prints in extract.py for other non-interpolated f-strings to remove
similarly.
- Around line 346-350: The default output filename currently sets output_path to
LOCALES_DIR / "_template.json" when args.locale is not provided; change that to
use the documented template name by setting output_path to LOCALES_DIR /
"en_template.json" so the behavior of the args.locale branch (and the
output_path variable) matches the translator workflow and downstream copy steps;
update the logic around args.locale/output_path in extract.py accordingly
(references: args.locale, output_path, LOCALES_DIR).
- Around line 22-23: The script tools/i18n/extract.py imports BeautifulSoup,
Comment, and NavigableString from bs4 but there is no declared dependency; add
beautifulsoup4 to the repository's tooling requirements (e.g., append to the
main requirements.txt or create a dedicated tools/i18n/requirements.txt or
requirements-i18n.txt) and update any i18n setup docs or README with the pip
install step (pip install beautifulsoup4 or pip install -r <i18n-reqs>) so
running extract.py in a clean environment won't raise ImportError.
---
Nitpick comments:
In `@tools/i18n/ARCHITECTURE.md`:
- Around line 7-23: Update the fenced ASCII diagram blocks so they include a
language tag (e.g., use ```text instead of ```) to satisfy markdownlint MD040;
specifically edit the fences that wrap the diagrams showing "Core Repo
(WLED/tools/i18n/)" and "Translation Repo (WLED-translations/<locale>/)" and the
other ASCII/structure blocks referenced (the ones with the box characters and
lists like extract.py, build.py, static.json, js.json, effects.json,
palettes.json, metadata.json) to use a language identifier such as text for each
opening fence.
In `@tools/i18n/README.md`:
- Around line 7-19: The fenced code blocks in tools/i18n/README.md are missing
language labels which triggers MD040; update each triple-backtick block (the
ASCII flow diagrams around the extract/build steps and the similar block later)
by adding a language identifier such as text (i.e., change ``` to ```text) so
the markdown linter recognizes the fence language; ensure you update both
occurrences referenced in the comment so all fenced blocks include the label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4d90e7e-f8a3-4d80-ba6b-be9b6b3b517f
📒 Files selected for processing (11)
tools/i18n/ARCHITECTURE.mdtools/i18n/README.mdtools/i18n/build.pytools/i18n/extract.pytools/i18n/locales/en_template.jsonwled00/data/settings_dmx.htmwled00/data/settings_leds.htmwled00/data/settings_sec.htmwled00/data/settings_sync.htmwled00/data/settings_wifi.htmwled00/data/update.htm
✅ Files skipped from review due to trivial changes (4)
- wled00/data/settings_sec.htm
- wled00/data/settings_dmx.htm
- wled00/data/settings_sync.htm
- tools/i18n/locales/en_template.json
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/data/update.htm
- wled00/data/settings_wifi.htm
- wled00/data/settings_leds.htm
| print(f" Searched:", file=sys.stderr) | ||
| print(f" --translations-dir (if provided)", file=sys.stderr) |
There was a problem hiding this comment.
Remove non-interpolated f-strings flagged by Ruff F541.
These strings don’t interpolate values; keeping f prefixes will fail lint in strict setups.
Also applies to: 274-275
🧰 Tools
🪛 Ruff (0.15.15)
[error] 96-96: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 97-97: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/build.py` around lines 96 - 97, Remove the unnecessary f-string
prefixes from the static print statements so Ruff F541 is not triggered: change
the two prints shown (print(f" Searched:", file=sys.stderr) and print(f"
--translations-dir (if provided)", file=sys.stderr)) to plain string prints (no
leading f), and make the same change for the other occurrences flagged (the
analogous prints at the later section referenced around the second pair). Locate
and update the exact print calls in build.py (the static stderr print
statements) to remove the "f" prefix while leaving the rest of the call
unchanged.
Source: Linters/SAST tools
| files_to_load = sorted(tdir.glob("*.json")) | ||
| single = tdir / f"{locale}.json" | ||
| if single.exists() and single not in files_to_load: | ||
| files_to_load = [single] | ||
| else: |
There was a problem hiding this comment.
Local fallback can unintentionally merge multiple locale files.
When tdir is tools/i18n/locales/, this loads all *.json even if <locale>.json exists, so entries from other locale files can bleed into output.
Suggested fix
- elif tdir.is_dir():
- files_to_load = sorted(tdir.glob("*.json"))
- single = tdir / f"{locale}.json"
- if single.exists() and single not in files_to_load:
- files_to_load = [single]
+ elif tdir.is_dir():
+ single = tdir / f"{locale}.json"
+ if single.exists():
+ files_to_load = [single]
+ else:
+ files_to_load = sorted(tdir.glob("*.json"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/build.py` around lines 108 - 112, The current logic in build.py
first sets files_to_load = sorted(tdir.glob("*.json")) and only replaces it with
the single file when single.exists() and single not in files_to_load, which
means when single is present alongside other *.json files (e.g., in
tools/i18n/locales/) other locale files still get loaded; change the branch so
that if the specific locale file (single) exists you set files_to_load =
[single] unconditionally (or filter files_to_load to only include single)
instead of only when it's not already in the glob list, referencing the
variables/files_to_load, tdir, single, and locale to locate the change.
| trans = entry.get('translation', '').strip() | ||
| if trans: | ||
| merged[fname][key] = { | ||
| 'original': entry.get('en', ''), | ||
| 'translation': trans, | ||
| } |
There was a problem hiding this comment.
--validate can report false success because untranslated keys are dropped at load time.
load_translations() filters out empty translation entries, then validate_translations() measures completeness only on that filtered set. Missing translations are therefore invisible.
Suggested fix
-def load_translations(locale, translations_dir=None):
+def load_translations(locale, translations_dir=None, include_untranslated=False):
@@
- trans = entry.get('translation', '').strip()
- if trans:
- merged[fname][key] = {
- 'original': entry.get('en', ''),
- 'translation': trans,
- }
+ trans = entry.get('translation', '').strip()
+ if trans or include_untranslated:
+ merged[fname][key] = {
+ 'original': entry.get('en', ''),
+ 'translation': trans,
+ }
-def validate_translations(locale, translations_dir=None):
+def validate_translations(locale, translations_dir=None):
@@
- file_translations = load_translations(locale, translations_dir)
+ file_translations = load_translations(locale, translations_dir, include_untranslated=True)Also applies to: 299-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/build.py` around lines 125 - 130, load_translations() currently
drops entries with empty entry['translation'], causing validate_translations()
to only see translated keys and report false success; change load_translations()
(the block that builds merged[fname][key]) to always include keys from the
source (store translation as '' or None when missing) so merged retains every
original key, and update validate_translations() to compute completeness against
the full set in merged (checking translation==''/None as missing). Ensure
references: function names load_translations(), validate_translations(), the
merged dict, and the entry {'translation','en'} handling are updated so
untranslated keys are preserved and counted as missing.
| from bs4 import BeautifulSoup, Comment | ||
| from bs4.element import NavigableString |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find BeautifulSoup usage =="
rg -n "from bs4|import bs4|BeautifulSoup" tools/i18n
echo
echo "== Find dependency declarations =="
fd -HI "requirements*.txt|pyproject.toml|Pipfile|poetry.lock|setup.py|README.md" . | sed 's#^\./##'
echo
echo "== Search manifests/docs for beautifulsoup4/bs4 =="
rg -n "beautifulsoup4|\\bbs4\\b" \
pyproject.toml setup.py Pipfile poetry.lock README.md tools/i18n/README.md requirements.txt requirements-dev.txt 2>/dev/null || trueRepository: wled/WLED
Length of output: 1117
Declare beautifulsoup4/bs4 as a tooling dependency for tools/i18n/extract.py (lines 22-23).
tools/i18n/extract.py imports bs4 (BeautifulSoup, Comment, NavigableString), but the repo’s dependency/docs search shows no beautifulsoup4/bs4 entry in the checked dependency manifests (e.g., requirements.txt) or i18n docs—so running the script in a clean environment will raise ImportError. Add beautifulsoup4 to the tooling requirements (or create a dedicated i18n requirements file) and document the install step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/extract.py` around lines 22 - 23, The script tools/i18n/extract.py
imports BeautifulSoup, Comment, and NavigableString from bs4 but there is no
declared dependency; add beautifulsoup4 to the repository's tooling requirements
(e.g., append to the main requirements.txt or create a dedicated
tools/i18n/requirements.txt or requirements-i18n.txt) and update any i18n setup
docs or README with the pip install step (pip install beautifulsoup4 or pip
install -r <i18n-reqs>) so running extract.py in a clean environment won't raise
ImportError.
| if result['coverage'] < 100: | ||
| print(f"\nFAILED: Coverage is {result['coverage']:.1f}%, expected 100%") | ||
| sys.exit(1) | ||
| print(f"\nPASSED: All strings translated") |
There was a problem hiding this comment.
Remove the empty f prefix to satisfy Ruff F541.
This f-string has no interpolation and is flagged by lint.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 325-325: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/extract.py` at line 325, The print call uses an unnecessary
f-string (print(f"\nPASSED: All strings translated")) which triggers Ruff F541;
change it to a plain string by removing the leading f (use print("\nPASSED: All
strings translated")) and scan nearby prints in extract.py for other
non-interpolated f-strings to remove similarly.
Source: Linters/SAST tools
| elif args.locale: | ||
| output_path = LOCALES_DIR / f"{args.locale}.json" | ||
| else: | ||
| output_path = LOCALES_DIR / "_template.json" | ||
|
|
There was a problem hiding this comment.
Default output filename breaks the documented template contract.
When --locale is omitted, this writes _template.json, but the workflow/baseline uses en_template.json. That mismatch breaks translator quick-start paths and downstream copy steps.
Suggested fix
- else:
- output_path = LOCALES_DIR / "_template.json"
+ else:
+ output_path = LOCALES_DIR / "en_template.json"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif args.locale: | |
| output_path = LOCALES_DIR / f"{args.locale}.json" | |
| else: | |
| output_path = LOCALES_DIR / "_template.json" | |
| elif args.locale: | |
| output_path = LOCALES_DIR / f"{args.locale}.json" | |
| else: | |
| output_path = LOCALES_DIR / "en_template.json" | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/extract.py` around lines 346 - 350, The default output filename
currently sets output_path to LOCALES_DIR / "_template.json" when args.locale is
not provided; change that to use the documented template name by setting
output_path to LOCALES_DIR / "en_template.json" so the behavior of the
args.locale branch (and the output_path variable) matches the translator
workflow and downstream copy steps; update the logic around
args.locale/output_path in extract.py accordingly (references: args.locale,
output_path, LOCALES_DIR).
This isn't 100% true - otherwise the esp01 1MB build would not fail. But I agree its an efficient solution. |
|
|
|
|
|
I think the scripts need a "diff" capability, so translations can be maintained when a new WLED release comes out. This is important for selection lists, which seem to be translated "by item order in the list". When the list is changed - by inserting an item between the existing ones - it could be that translations are becoming "off-by-one" - the 4th selection item may be a different one, and the old translation text now needs to be attached to the 5th item 🤔 (that was item 4 in the previous WLED release). Example: ethernet drop-down WLED/wled00/data/settings_wifi.htm Lines 231 to 247 in 30abc01 |
Is this for in the main WLED repo, or is it a "local patch" in the translation fork? Collecting |
I don't understand - how did you solve all these points? I thought we were going to accept most of them as known limitations? |
To be transparent, we should add that the approach in this PR cannot handle:
|
actually only the external add-ons are not translateable, palette editor, file system editor and pixelforge I all put in subfolders but they are local source. |
The HTM file changes should not be in the main WLED repo. Translation files belong in the separate WLED-translations repository. This PR now only contains the i18n toolchain (extract.py, build.py).
- Add diff.py: compare template versions to find added/removed/modified strings - Update README: fix 'zero flash overhead' claim, add Known Limitations section - Update README: add diff.py usage, fix JS string count (45→716) - Remove AI slop, simplify language
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
tools/i18n/extract.py (3)
325-325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unnecessary f-string prefix.
Line 325 has no interpolation. Change
print(f"\nPASSED: All strings translated")toprint("\nPASSED: All strings translated").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/extract.py` at line 325, Replace the unnecessary f-string on the print call by removing the 'f' prefix: change the print invocation `print(f"\nPASSED: All strings translated")` to a normal string literal `print("\nPASSED: All strings translated")` so there is no needless interpolation; update the print statement in tools/i18n/extract.py where that print(...) call appears.Source: Linters/SAST tools
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing dependency:
beautifulsoup4not declared.The script imports
BeautifulSoup,Comment, andNavigableStringfrombs4, but there's nobeautifulsoup4dependency in any requirements file. Runningextract.pyin a clean environment will raiseImportError.🔧 Suggested fix
Add
beautifulsoup4to a requirements file (e.g.,tools/i18n/requirements.txt) and document the install step in README.md:# In tools/i18n/requirements.txt beautifulsoup4>=4.12.0Update README.md to include:
pip install -r tools/i18n/requirements.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/extract.py` around lines 22 - 23, The extract.py script imports BeautifulSoup, Comment, and NavigableString from bs4 but the project lacks the beautifulsoup4 dependency; add "beautifulsoup4>=4.12.0" (or appropriate version) to the i18n tooling requirements file (e.g., tools/i18n/requirements.txt) and update the README installation instructions to include pip install -r tools/i18n/requirements.txt so running extract.py in a clean environment won't raise ImportError.
346-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault output filename breaks documented workflow.
Line 349 outputs
_template.json, but the documented workflow and README expecten_template.json. This mismatch breaks translator quick-start paths.🔧 Suggested fix
else: - output_path = LOCALES_DIR / "_template.json" + output_path = LOCALES_DIR / "en_template.json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/extract.py` around lines 346 - 350, The default output filename is set to "_template.json" which conflicts with the README expecting "en_template.json"; update the logic that sets output_path (the branch using args.locale and LOCALES_DIR) so the default when args.locale is not provided is LOCALES_DIR / "en_template.json" instead of "_template.json". Ensure references in the script that use output_path (e.g., any later calls that write or log the output file) continue to work with the new filename.tools/i18n/build.py (3)
96-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unnecessary f-string prefixes.
Lines 96-97 (and similar lines 274-275) use f-strings without interpolation. Remove the
fprefix to satisfy Ruff F541.🔧 Suggested fix
- print(f" Searched:", file=sys.stderr) - print(f" --translations-dir (if provided)", file=sys.stderr) + print(" Searched:", file=sys.stderr) + print(" --translations-dir (if provided)", file=sys.stderr)Apply the same fix to lines 274-275.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/build.py` around lines 96 - 99, Remove the unnecessary f-string prefixes on plain print calls in build.py: replace lines that call print(f"...", file=sys.stderr) with print("...", file=sys.stderr) for the occurrences shown (the block printing search locations around the Searched header and the similar block at lines ~274-275). Update the print statements in the function or top-level section where "Searched:" and the subsequent path lines are printed so they use regular string literals (no interpolation) to satisfy Ruff F541.Source: Linters/SAST tools
107-112:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Locale file collision can cause cross-locale bleed.
When
tdiris a directory (e.g.,tools/i18n/locales/), the code loads ALL*.jsonfiles first, then only conditionally replaces with a single file if it exists AND isn't already in the list. This means multiple locale files can be merged together, corrupting the output.🐛 Proposed fix
elif tdir.is_dir(): - files_to_load = sorted(tdir.glob("*.json")) single = tdir / f"{locale}.json" - if single.exists() and single not in files_to_load: + if single.exists(): files_to_load = [single] + else: + files_to_load = sorted(tdir.glob("*.json"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/build.py` around lines 107 - 112, The code in the tdir branch can merge multiple locale JSONs because files_to_load is set to all *.json and only replaced with the single file when that single isn't present in the list; instead, if the per-locale file (single = tdir / f"{locale}.json") exists you must load only that file to avoid cross-locale bleed. Update the logic around tdir, files_to_load and single so that if single.exists() then files_to_load = [single] (unconditionally), otherwise fall back to sorted(tdir.glob("*.json")); keep the rest of the flow unchanged.
125-130:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical:
--validatereports false success by hiding untranslated keys.
load_translations()filters out entries with emptytranslationfields (line 126), sovalidate_translations()only sees the translated subset and incorrectly reports 100% coverage even when translations are incomplete.🔧 Suggested fix
Change
load_translations()to preserve all keys regardless of translation status, and updatevalidate_translations()to count empty translations as missing:-def load_translations(locale, translations_dir=None): +def load_translations(locale, translations_dir=None, include_untranslated=False): """Load translation JSON for the given locale, keyed by file.""" ... for key, entry in entries.items(): trans = entry.get('translation', '').strip() - if trans: + if trans or include_untranslated: merged[fname][key] = { 'original': entry.get('en', ''), 'translation': trans, }Then in
validate_translations():def validate_translations(locale, translations_dir=None): """Validate translation completeness against English source files.""" - file_translations = load_translations(locale, translations_dir) + file_translations = load_translations(locale, translations_dir, include_untranslated=True) ... for fname, entries in file_translations.items(): - translated = sum(1 for e in entries.values() if e.get('translation', '').strip()) + translated = sum(1 for e in entries.values() if e.get('translation', ''))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/build.py` around lines 125 - 130, load_translations() currently drops entries where entry.get('translation') is empty, causing validate_translations() to see only translated keys; change load_translations() (the block that builds merged[fname][key]) to always add the key (use entry.get('translation', '').strip() but do not skip when empty) so merged contains all keys with 'translation' set to '' when untranslated, and then update validate_translations() to treat empty strings as missing (i.e., count entries where merged[fname][key]['translation'] == '' as untranslated/missing) so coverage is computed correctly.
🧹 Nitpick comments (3)
tools/i18n/diff.py (1)
90-117: ⚡ Quick winExtract duplicate template loading logic into a helper function.
The old and new template loading blocks (lines 94-104 and 107-117) are nearly identical, differing only in variable names. Extract this logic into a helper function to improve maintainability and reduce the risk of divergence if the loading logic changes.
♻️ Proposed refactor to eliminate duplication
+def load_templates(path, locale=None): + """Load templates from file or directory with optional locale subdirectory.""" + path = Path(path) + if path.is_dir(): + if locale: + locale_dir = path / locale + if locale_dir.exists(): + return load_templates_from_dir(locale_dir) + else: + return load_templates_from_dir(path) + else: + return load_templates_from_dir(path) + else: + return load_template(path) + def main(): import argparse parser = argparse.ArgumentParser( description='Diff WLED i18n templates between versions' ) parser.add_argument( '--old', required=True, help='Old template JSON file or directory' ) parser.add_argument( '--new', required=True, help='New template JSON file or directory' ) parser.add_argument( '--locale', help='Locale to compare (e.g., zh_CN). Only used with directory mode.' ) args = parser.parse_args() - old_path = Path(args.old) - new_path = Path(args.new) - - # Load old templates - if old_path.is_dir(): - if args.locale: - locale_dir = old_path / args.locale - if locale_dir.exists(): - old = load_templates_from_dir(locale_dir) - else: - old = load_templates_from_dir(old_path) - else: - old = load_templates_from_dir(old_path) - else: - old = load_template(old_path) - - # Load new templates - if new_path.is_dir(): - if args.locale: - locale_dir = new_path / args.locale - if locale_dir.exists(): - new = load_templates_from_dir(locale_dir) - else: - new = load_templates_from_dir(new_path) - else: - new = load_templates_from_dir(new_path) - else: - new = load_template(new_path) + old = load_templates(args.old, args.locale) + new = load_templates(args.new, args.locale) # Compute diff result = diff_templates(old, new)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/diff.py` around lines 90 - 117, Extract the duplicated logic that loads templates for a path into a helper function (e.g., load_templates_for_path(path, args)), which should accept a Path and args.locale and return either a dict/list from load_templates_from_dir or a single template from load_template; replace the two blocks that use old_path/new_path with calls to this helper (old = load_templates_for_path(old_path, args) and new = load_templates_for_path(new_path, args)), preserving the existing behavior of checking is_dir(), using locale subdirectory when args.locale is set and exists, and falling back to the parent directory when it does not.tools/i18n/extract.py (1)
254-254: 💤 Low valueOptional: Rename unused loop variables.
Ruff flags
fname(line 254) andinfo(line 266) as unused loop variables. Consider prefixing with underscore (_fname,_info) to signal intent.Also applies to: 266-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/extract.py` at line 254, The loop variables flagged as unused should be renamed with a leading underscore to signal intentional unused usage: change the comprehension "for fname, entries in locale_data.items()" to use "_fname" instead of "fname", and similarly rename "info" to "_info" wherever it's only present to satisfy Ruff (e.g., the loop using "info" at the later occurrence); update all references in those loop headers only, leaving the rest of the logic unchanged.Source: Linters/SAST tools
tools/i18n/ARCHITECTURE.md (1)
7-23: 💤 Low valueOptional: Add language hints to fenced code blocks.
Markdownlint flags these blocks as missing language specifiers. Consider adding
textor leaving empty triple-backticks for ASCII diagrams to satisfy the linter.Also applies to: 67-75, 112-120, 124-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/i18n/ARCHITECTURE.md` around lines 7 - 23, Update the fenced code blocks in ARCHITECTURE.md that contain ASCII diagrams (the large repository diagrams and the similar blocks at the other noted sections) to include a language hint—either add "text" after the opening triple backticks or use empty triple-backticks—to satisfy markdownlint; locate the ASCII diagram blocks (the repo structure boxes and the other fenced diagrams) and change their opening fences from ``` to ```text or ``` to silence the linter.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/i18n/ARCHITECTURE.md`:
- Line 18: The Layer 2 js.json entry counts are inconsistent across docs and the
actual template: reconcile and correct the documentation by (A) counting unique
keys prefixed with "js:" in tools/i18n/locales/en_template.json (current count =
43) and then update the count cited in tools/i18n/ARCHITECTURE.md and
tools/i18n/README.md to that number, or (B) if the 716 figure comes from a
different metric, explicitly document the counting method and source (e.g.,
combined keys across locales vs unique template keys) in both files; ensure you
reference the files/tools mentioned (tools/i18n/locales/en_template.json,
tools/i18n/ARCHITECTURE.md, tools/i18n/README.md, js.json) so the change is
applied consistently.
In `@tools/i18n/build.py`:
- Around line 272-276: The current check in the out_dir == src_dir block only
prints warnings and still proceeds, risking data loss; change the behavior in
the conditional that compares out_dir and src_dir (the block using
out_dir.resolve() == src_dir.resolve()) to abort the process immediately with a
non-zero exit (e.g., call sys.exit(1) or raise SystemExit) after printing the
message, so the script fails fast instead of continuing and overwriting source
files.
In `@tools/i18n/diff.py`:
- Around line 94-102: When args.locale is provided but the computed locale_dir
(old_path / args.locale) does not exist, emit a clear warning before falling
back to load_templates_from_dir(old_path); currently the code silently falls
back. Update the branch in diff.py that references old_path, args.locale,
locale_dir, load_templates_from_dir and old to print or log a warning (e.g.,
logger.warning or print to stderr) including the requested locale name, then
proceed to call load_templates_from_dir(old_path); make the same change for the
similar block that uses these same symbols around the other occurrence (the
107-115 block).
- Around line 37-68: The diff_templates function assumes each template value is
a dict with an 'en' key; add validation at the start of the function (and before
comparing common keys) to ensure each value in both old and new is a mapping and
contains an 'en' string value. In diff_templates validate entries for old[k] and
new[k] (and when building old_keys/new_keys) and if any entry is not a dict or
its 'en' is missing or not a string, raise a clear ValueError (or aggregate and
raise one) referencing the offending template key(s) and whether they came from
old or new so the caller can fix the malformed templates before diffing. Ensure
the validation mentions function name diff_templates and fields 'en' and key
names in the error message.
- Around line 25-34: The current load_templates_from_dir silently overwrites
keys when merging JSON files; modify load_templates_from_dir to detect
collisions before merging by computing duplicates = set(result.keys()) &
set(data.keys()) for each json_file (loaded via load_template), and if
duplicates is non-empty either log a clear warning or raise an exception (e.g.,
ValueError) that includes the duplicate key names and the json_file.name so
conflicts are reported instead of silently overwritten; only call
result.update(data) when no collisions are found.
- Around line 25-34: In load_templates_from_dir, ensure we validate that at
least one JSON template (besides metadata.json) was loaded: after iterating and
populating result, check if result is empty and if so raise a clear exception
(e.g., ValueError) that includes the dir_path so callers know the directory had
no template files; this check should be placed in load_templates_from_dir just
before the return to prevent returning an empty dict.
- Around line 19-22: The load_template function lacks error handling for file
I/O and JSON parsing; wrap the open(...) and json.load(f) calls in a try/except
that catches FileNotFoundError, PermissionError, and json.JSONDecodeError (and a
generic Exception fallback), and raise or log a new descriptive error that
includes the path and the original exception message so callers (e.g.,
directory-mode callers) can identify which file failed; reference the
load_template function and json.load to locate where to add the try/except and
include the path in the error text.
---
Duplicate comments:
In `@tools/i18n/build.py`:
- Around line 96-99: Remove the unnecessary f-string prefixes on plain print
calls in build.py: replace lines that call print(f"...", file=sys.stderr) with
print("...", file=sys.stderr) for the occurrences shown (the block printing
search locations around the Searched header and the similar block at lines
~274-275). Update the print statements in the function or top-level section
where "Searched:" and the subsequent path lines are printed so they use regular
string literals (no interpolation) to satisfy Ruff F541.
- Around line 107-112: The code in the tdir branch can merge multiple locale
JSONs because files_to_load is set to all *.json and only replaced with the
single file when that single isn't present in the list; instead, if the
per-locale file (single = tdir / f"{locale}.json") exists you must load only
that file to avoid cross-locale bleed. Update the logic around tdir,
files_to_load and single so that if single.exists() then files_to_load =
[single] (unconditionally), otherwise fall back to sorted(tdir.glob("*.json"));
keep the rest of the flow unchanged.
- Around line 125-130: load_translations() currently drops entries where
entry.get('translation') is empty, causing validate_translations() to see only
translated keys; change load_translations() (the block that builds
merged[fname][key]) to always add the key (use entry.get('translation',
'').strip() but do not skip when empty) so merged contains all keys with
'translation' set to '' when untranslated, and then update
validate_translations() to treat empty strings as missing (i.e., count entries
where merged[fname][key]['translation'] == '' as untranslated/missing) so
coverage is computed correctly.
In `@tools/i18n/extract.py`:
- Line 325: Replace the unnecessary f-string on the print call by removing the
'f' prefix: change the print invocation `print(f"\nPASSED: All strings
translated")` to a normal string literal `print("\nPASSED: All strings
translated")` so there is no needless interpolation; update the print statement
in tools/i18n/extract.py where that print(...) call appears.
- Around line 22-23: The extract.py script imports BeautifulSoup, Comment, and
NavigableString from bs4 but the project lacks the beautifulsoup4 dependency;
add "beautifulsoup4>=4.12.0" (or appropriate version) to the i18n tooling
requirements file (e.g., tools/i18n/requirements.txt) and update the README
installation instructions to include pip install -r tools/i18n/requirements.txt
so running extract.py in a clean environment won't raise ImportError.
- Around line 346-350: The default output filename is set to "_template.json"
which conflicts with the README expecting "en_template.json"; update the logic
that sets output_path (the branch using args.locale and LOCALES_DIR) so the
default when args.locale is not provided is LOCALES_DIR / "en_template.json"
instead of "_template.json". Ensure references in the script that use
output_path (e.g., any later calls that write or log the output file) continue
to work with the new filename.
---
Nitpick comments:
In `@tools/i18n/ARCHITECTURE.md`:
- Around line 7-23: Update the fenced code blocks in ARCHITECTURE.md that
contain ASCII diagrams (the large repository diagrams and the similar blocks at
the other noted sections) to include a language hint—either add "text" after the
opening triple backticks or use empty triple-backticks—to satisfy markdownlint;
locate the ASCII diagram blocks (the repo structure boxes and the other fenced
diagrams) and change their opening fences from ``` to ```text or ``` to silence
the linter.
In `@tools/i18n/diff.py`:
- Around line 90-117: Extract the duplicated logic that loads templates for a
path into a helper function (e.g., load_templates_for_path(path, args)), which
should accept a Path and args.locale and return either a dict/list from
load_templates_from_dir or a single template from load_template; replace the two
blocks that use old_path/new_path with calls to this helper (old =
load_templates_for_path(old_path, args) and new =
load_templates_for_path(new_path, args)), preserving the existing behavior of
checking is_dir(), using locale subdirectory when args.locale is set and exists,
and falling back to the parent directory when it does not.
In `@tools/i18n/extract.py`:
- Line 254: The loop variables flagged as unused should be renamed with a
leading underscore to signal intentional unused usage: change the comprehension
"for fname, entries in locale_data.items()" to use "_fname" instead of "fname",
and similarly rename "info" to "_info" wherever it's only present to satisfy
Ruff (e.g., the loop using "info" at the later occurrence); update all
references in those loop headers only, leaving the rest of the logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2da8a536-5449-4d57-b2d0-eac62d0d5cb3
📒 Files selected for processing (6)
tools/i18n/ARCHITECTURE.mdtools/i18n/README.mdtools/i18n/build.pytools/i18n/diff.pytools/i18n/extract.pytools/i18n/locales/en_template.json
✅ Files skipped from review due to trivial changes (1)
- tools/i18n/locales/en_template.json
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ Translation Repo (WLED-translations/<locale>/) │ | ||
| │ ├── static.json # Layer 1: Static HTML (429 entries) │ | ||
| │ ├── js.json # Layer 2: JS strings (45 entries) │ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Count actual JS strings in the English template
if [ -f tools/i18n/locales/en_template.json ]; then
echo "Counting js: keys in en_template.json:"
rg -o '"js:[^"]+":' tools/i18n/locales/en_template.json | wc -l
else
echo "en_template.json not found; run extract.py first"
fiRepository: wled/WLED
Length of output: 95
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the referenced lines in both docs
echo "---- tools/i18n/ARCHITECTURE.md (lines 1-220) excerpts around 18, 32, 152 ----"
sed -n '10,40p' tools/i18n/ARCHITECTURE.md
sed -n '140,170p' tools/i18n/ARCHITECTURE.md
echo
echo "---- README.md (excerpts around lines 46 and 132) ----"
sed -n '35,60p' README.md
sed -n '120,150p' README.md
echo
# Find occurrences of the claimed counts
echo "---- Occurrences of '45 entries' and '716 entries' ----"
rg -n "45 entries|716 entries|Layer 2|JS strings" tools/i18n/ARCHITECTURE.md README.md || true
echo
# Compute js-string count by parsing JSON (authoritative)
if [ -f tools/i18n/locales/en_template.json ]; then
echo "---- Count js:* keys in tools/i18n/locales/en_template.json via JSON parsing ----"
python3 - <<'PY'
import json
from pathlib import Path
p = Path("tools/i18n/locales/en_template.json")
data = json.loads(p.read_text(encoding="utf-8"))
keys = list(data.keys()) if isinstance(data, dict) else []
js_keys = [k for k in keys if isinstance(k, str) and k.startswith("js:")]
print("Total keys:", len(keys))
print("js:* keys:", len(js_keys))
print("First 10 js keys:", js_keys[:10])
PY
else
echo "tools/i18n/locales/en_template.json not found"
fiRepository: wled/WLED
Length of output: 2021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Repo root files (top-level) ----"
ls -la
echo
echo "---- Find README* files ----"
fd -H --max-depth 4 "README" -t f . || true
# Also fall back to common names if fd misses
ls -la README* 2>/dev/null || true
echo
echo "---- Authoritative count: js:* keys in en_template.json ----"
python3 - <<'PY'
import json
from pathlib import Path
p = Path("tools/i18n/locales/en_template.json")
data = json.loads(p.read_text(encoding="utf-8"))
js_keys = [k for k in data.keys() if isinstance(k, str) and k.startswith("js:")]
print("tools/i18n/locales/en_template.json js:* keys:", len(js_keys))
PY
echo
echo "---- Search docs for '716 entries' and '45 entries' near translation coverage ----"
rg -n "716 entries|45 entries|Layer 2: JS strings|js\\.json" -S . || trueRepository: wled/WLED
Length of output: 3680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- tools/i18n/ARCHITECTURE.md: around claimed '45' ----"
sed -n '1,60p' tools/i18n/ARCHITECTURE.md
echo
sed -n '110,170p' tools/i18n/ARCHITECTURE.md
echo
echo "---- tools/i18n/README.md: around claimed '716' ----"
sed -n '35,70p' tools/i18n/README.md
echo
sed -n '110,160p' tools/i18n/README.md
echo
echo "---- en_template.json structure peek ----"
python3 - <<'PY'
import json
from pathlib import Path
p = Path("tools/i18n/locales/en_template.json")
txt = p.read_text(encoding="utf-8")
print("File bytes:", len(txt))
print("First 300 chars:\n", txt[:300].replace("\n","\\n"),"\n")
try:
data = json.loads(txt)
print("Top-level type:", type(data).__name__)
if isinstance(data, dict):
print("Top-level dict keys sample:", list(data.keys())[:10])
elif isinstance(data, list):
print("Top-level list length:", len(data))
except Exception as e:
print("json.loads failed:", e)
PY
echo
echo "---- en_template.json: count js:* occurrences (same logic as original comment) ----"
rg -o '"js:[^"]+":' tools/i18n/locales/en_template.json | wc -l
echo
echo "---- en_template.json: count unique js:* keys via regex extraction ----"
python3 - <<'PY'
import re
from pathlib import Path
txt = Path("tools/i18n/locales/en_template.json").read_text(encoding="utf-8")
keys = re.findall(r'"(js:[^"]+)"\s*:', txt)
print("Unique js:* keys:", len(set(keys)))
print("First 10 unique js keys:", sorted(set(keys))[:10])
PYRepository: wled/WLED
Length of output: 7763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search for count/stat logic in tools/i18n/*.py ----"
rg -n "JS strings|Layer 2|716|45|js\.json|en_template\.json|--stats" tools/i18n -S || true
echo
echo "---- Show extract.py relevant sections ----"
ls -ლა tools/i18n
python3 - <<'PY'
from pathlib import Path
p=Path("tools/i18n/extract.py")
print("extract.py exists:", p.exists(), "size:", p.stat().st_size if p.exists() else None)
PY
sed -n '1,220p' tools/i18n/extract.py
echo
echo "---- Show build.py relevant sections (first 260 lines) ----"
sed -n '1,260p' tools/i18n/build.py
echo
echo "---- Search for 'entries' formatting in docs/scripts ----"
rg -n "entries" tools/i18n -S || trueRepository: wled/WLED
Length of output: 2113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the extract.py stats implementation
sed -n '260,420p' tools/i18n/extract.py
echo
echo "---- extract.py: look for counting logic ----"
rg -n "stats|JS strings|716|45|js\.json|innerHTML|innerText|alert" tools/i18n/extract.py -S
echo
echo "---- extract.py: show nearby lines around JS-string stats marker ----"
rg -n "JS strings" tools/i18n/extract.py -n -S || trueRepository: wled/WLED
Length of output: 4408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Run the i18n extractor in stats mode to see how it counts "JS strings"
python3 tools/i18n/extract.py --stats | head -n 200
echo
echo "---- Additionally, regenerate _template.json and count js:* keys inside it ----"
python3 tools/i18n/extract.py --output tools/i18n/locales/_template_generated.json
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("tools/i18n/locales/_template_generated.json")
txt = p.read_text(encoding="utf-8")
keys = re.findall(r'"(js:[^"]+)"\s*:', txt)
print("Generated _template_generated.json unique js:* keys:", len(set(keys)))
print("Occurrences of js:* keys in JSON:", len(keys))
PYRepository: wled/WLED
Length of output: 2443
Fix inconsistent Layer 2 JS-string entry counts in i18n documentation
tools/i18n/ARCHITECTURE.mdstates Layer 2js.jsonhas 45 entries.tools/i18n/README.mdstates Layer 2js.jsonhas 716 entries.- The current English template (
tools/i18n/locales/en_template.json) contains 43 uniquejs:*keys.
Update both docs to the same count (or explicitly document the counting method if 716 comes from a different metric/source).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/ARCHITECTURE.md` at line 18, The Layer 2 js.json entry counts are
inconsistent across docs and the actual template: reconcile and correct the
documentation by (A) counting unique keys prefixed with "js:" in
tools/i18n/locales/en_template.json (current count = 43) and then update the
count cited in tools/i18n/ARCHITECTURE.md and tools/i18n/README.md to that
number, or (B) if the 716 figure comes from a different metric, explicitly
document the counting method and source (e.g., combined keys across locales vs
unique template keys) in both files; ensure you reference the files/tools
mentioned (tools/i18n/locales/en_template.json, tools/i18n/ARCHITECTURE.md,
tools/i18n/README.md, js.json) so the change is applied consistently.
| if out_dir.resolve() == src_dir.resolve(): | ||
| print(f"WARNING: Output dir equals source dir ({src_dir}).", file=sys.stderr) | ||
| print(f" English source files will be overwritten!", file=sys.stderr) | ||
| print(f" Pass --output-dir to a different location.", file=sys.stderr) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Warning: Output directory collision check is too weak.
The code warns but allows overwriting source files when out_dir == src_dir. This is a data loss hazard, especially for automated builds.
🛡️ Suggested improvement
Consider making this an error instead of a warning:
if out_dir.resolve() == src_dir.resolve():
- print(f"WARNING: Output dir equals source dir ({src_dir}).", file=sys.stderr)
- print(f" English source files will be overwritten!", file=sys.stderr)
- print(f" Pass --output-dir to a different location.", file=sys.stderr)
+ print(f"ERROR: Output dir equals source dir ({src_dir}).", file=sys.stderr)
+ print(" English source files would be overwritten!", file=sys.stderr)
+ print(" Pass --output-dir to a different location.", file=sys.stderr)
+ sys.exit(1)🧰 Tools
🪛 Ruff (0.15.15)
[error] 274-274: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 275-275: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/build.py` around lines 272 - 276, The current check in the out_dir
== src_dir block only prints warnings and still proceeds, risking data loss;
change the behavior in the conditional that compares out_dir and src_dir (the
block using out_dir.resolve() == src_dir.resolve()) to abort the process
immediately with a non-zero exit (e.g., call sys.exit(1) or raise SystemExit)
after printing the message, so the script fails fast instead of continuing and
overwriting source files.
| def load_template(path): | ||
| """Load a single JSON template file.""" | ||
| with open(path, encoding='utf-8') as f: | ||
| return json.load(f) |
There was a problem hiding this comment.
Add error handling for file I/O and JSON parsing failures.
The function will crash with unhelpful tracebacks if the file doesn't exist, contains invalid JSON, or is unreadable. In directory mode, users won't know which file failed. Add try/except with descriptive error messages.
🛡️ Proposed fix to add error handling
def load_template(path):
"""Load a single JSON template file."""
- with open(path, encoding='utf-8') as f:
- return json.load(f)
+ try:
+ with open(path, encoding='utf-8') as f:
+ return json.load(f)
+ except FileNotFoundError:
+ print(f"Error: Template file not found: {path}", file=sys.stderr)
+ sys.exit(1)
+ except json.JSONDecodeError as e:
+ print(f"Error: Invalid JSON in {path}: {e}", file=sys.stderr)
+ sys.exit(1)
+ except Exception as e:
+ print(f"Error: Failed to load {path}: {e}", file=sys.stderr)
+ sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/diff.py` around lines 19 - 22, The load_template function lacks
error handling for file I/O and JSON parsing; wrap the open(...) and
json.load(f) calls in a try/except that catches FileNotFoundError,
PermissionError, and json.JSONDecodeError (and a generic Exception fallback),
and raise or log a new descriptive error that includes the path and the original
exception message so callers (e.g., directory-mode callers) can identify which
file failed; reference the load_template function and json.load to locate where
to add the try/except and include the path in the error text.
| def load_templates_from_dir(dir_path): | ||
| """Load all JSON files from a directory and merge into single dict.""" | ||
| result = {} | ||
| dir_path = Path(dir_path) | ||
| for json_file in sorted(dir_path.glob('*.json')): | ||
| if json_file.name == 'metadata.json': | ||
| continue | ||
| data = load_template(json_file) | ||
| result.update(data) | ||
| return result |
There was a problem hiding this comment.
Detect and report key collisions between template files.
When multiple JSON files in the directory define the same key, dict.update() silently overwrites earlier values with later ones. This hides translation conflicts and can cause unexpected behavior. Add collision detection to warn or error when duplicate keys are found across files.
🔍 Proposed fix to detect collisions
def load_templates_from_dir(dir_path):
"""Load all JSON files from a directory and merge into single dict."""
result = {}
dir_path = Path(dir_path)
for json_file in sorted(dir_path.glob('*.json')):
if json_file.name == 'metadata.json':
continue
data = load_template(json_file)
+ # Detect key collisions
+ collisions = set(result.keys()) & set(data.keys())
+ if collisions:
+ print(f"Warning: Key collision in {json_file.name}: {collisions}", file=sys.stderr)
result.update(data)
return result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/diff.py` around lines 25 - 34, The current load_templates_from_dir
silently overwrites keys when merging JSON files; modify load_templates_from_dir
to detect collisions before merging by computing duplicates = set(result.keys())
& set(data.keys()) for each json_file (loaded via load_template), and if
duplicates is non-empty either log a clear warning or raise an exception (e.g.,
ValueError) that includes the duplicate key names and the json_file.name so
conflicts are reported instead of silently overwritten; only call
result.update(data) when no collisions are found.
Validate that at least one template was loaded from the directory.
If the directory contains no JSON files (or only metadata.json), the function returns an empty dict. This causes confusing diff results where all keys appear as added or removed. Add validation to ensure at least one template file was successfully loaded.
✅ Proposed fix to validate non-empty result
def load_templates_from_dir(dir_path):
"""Load all JSON files from a directory and merge into single dict."""
result = {}
dir_path = Path(dir_path)
for json_file in sorted(dir_path.glob('*.json')):
if json_file.name == 'metadata.json':
continue
data = load_template(json_file)
result.update(data)
+ if not result:
+ print(f"Error: No template files found in {dir_path}", file=sys.stderr)
+ sys.exit(1)
return result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/diff.py` around lines 25 - 34, In load_templates_from_dir, ensure
we validate that at least one JSON template (besides metadata.json) was loaded:
after iterating and populating result, check if result is empty and if so raise
a clear exception (e.g., ValueError) that includes the dir_path so callers know
the directory had no template files; this check should be placed in
load_templates_from_dir just before the return to prevent returning an empty
dict.
| def diff_templates(old, new): | ||
| """Compare two template dicts and return differences.""" | ||
| old_keys = set(old.keys()) | ||
| new_keys = set(new.keys()) | ||
|
|
||
| added = new_keys - old_keys | ||
| removed = old_keys - new_keys | ||
| common = old_keys & new_keys | ||
|
|
||
| modified = [] | ||
| for k in sorted(common): | ||
| old_en = old[k].get('en', '') | ||
| new_en = new[k].get('en', '') | ||
| if old_en != new_en: | ||
| modified.append({ | ||
| 'key': k, | ||
| 'old': old_en, | ||
| 'new': new_en, | ||
| }) | ||
|
|
||
| return { | ||
| 'added': sorted(added), | ||
| 'removed': sorted(removed), | ||
| 'modified': modified, | ||
| 'stats': { | ||
| 'old_count': len(old_keys), | ||
| 'new_count': len(new_keys), | ||
| 'added': len(added), | ||
| 'removed': len(removed), | ||
| 'modified': len(modified), | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate that template entries have the expected structure.
The function assumes each template value is a dict with an 'en' key, but doesn't validate this. If a template file has an unexpected structure (e.g., {"key": "string"} instead of {"key": {"en": "string"}}), the .get('en', '') silently returns an empty string, causing keys to appear as modified or creating confusing diff results. Add validation to ensure entries have the expected nested dict structure.
🛡️ Proposed fix to validate template structure
def diff_templates(old, new):
"""Compare two template dicts and return differences."""
+ # Validate structure
+ for name, template in [('old', old), ('new', new)]:
+ for key, value in template.items():
+ if not isinstance(value, dict):
+ print(f"Error: {name} template key '{key}' has invalid structure (expected dict, got {type(value).__name__})", file=sys.stderr)
+ sys.exit(1)
+ if 'en' not in value:
+ print(f"Warning: {name} template key '{key}' missing 'en' field", file=sys.stderr)
+
old_keys = set(old.keys())
new_keys = set(new.keys())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/diff.py` around lines 37 - 68, The diff_templates function assumes
each template value is a dict with an 'en' key; add validation at the start of
the function (and before comparing common keys) to ensure each value in both old
and new is a mapping and contains an 'en' string value. In diff_templates
validate entries for old[k] and new[k] (and when building old_keys/new_keys) and
if any entry is not a dict or its 'en' is missing or not a string, raise a clear
ValueError (or aggregate and raise one) referencing the offending template
key(s) and whether they came from old or new so the caller can fix the malformed
templates before diffing. Ensure the validation mentions function name
diff_templates and fields 'en' and key names in the error message.
| if old_path.is_dir(): | ||
| if args.locale: | ||
| locale_dir = old_path / args.locale | ||
| if locale_dir.exists(): | ||
| old = load_templates_from_dir(locale_dir) | ||
| else: | ||
| old = load_templates_from_dir(old_path) | ||
| else: | ||
| old = load_templates_from_dir(old_path) |
There was a problem hiding this comment.
Warn user when locale subdirectory doesn't exist.
When --locale is specified but the locale subdirectory doesn't exist, the code silently falls back to the base directory. This could confuse users who expect to compare locale-specific translations but unknowingly get a different template set. Add a warning message when falling back.
💬 Proposed fix to add fallback warning
# Load old templates
if old_path.is_dir():
if args.locale:
locale_dir = old_path / args.locale
if locale_dir.exists():
old = load_templates_from_dir(locale_dir)
else:
+ print(f"Warning: Locale directory {locale_dir} not found, using {old_path}", file=sys.stderr)
old = load_templates_from_dir(old_path)
else:
old = load_templates_from_dir(old_path)
else:
old = load_template(old_path)
# Load new templates
if new_path.is_dir():
if args.locale:
locale_dir = new_path / args.locale
if locale_dir.exists():
new = load_templates_from_dir(locale_dir)
else:
+ print(f"Warning: Locale directory {locale_dir} not found, using {new_path}", file=sys.stderr)
new = load_templates_from_dir(new_path)
else:
new = load_templates_from_dir(new_path)
else:
new = load_template(new_path)Also applies to: 107-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/i18n/diff.py` around lines 94 - 102, When args.locale is provided but
the computed locale_dir (old_path / args.locale) does not exist, emit a clear
warning before falling back to load_templates_from_dir(old_path); currently the
code silently falls back. Update the branch in diff.py that references old_path,
args.locale, locale_dir, load_templates_from_dir and old to print or log a
warning (e.g., logger.warning or print to stderr) including the requested locale
name, then proceed to call load_templates_from_dir(old_path); make the same
change for the similar block that uses these same symbols around the other
occurrence (the 107-115 block).
3 participants
Description
Complete Chinese (Simplified) translation of all 22 WLED Web UI HTML files.
Changes
lang="en"tolang="zh"in all filesTranslated Files (22 total)
Summary by CodeRabbit
Documentation
New Features