Skip to content

feat: add package.nls.json checking to find-missing-translations script #8255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mrubens merged 5 commits into from
Sep 23, 2025
Merged

feat: add package.nls.json checking to find-missing-translations script #8255

mrubens merged 5 commits into from
Sep 23, 2025

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Sep 23, 2025 •
edited by ellipsis-dev bot
Loading

This PR addresses the issue where the find-missing-translations script was not checking the package.nls.json files for missing translations.

Changes Made

  • Added a new "package-nls" area option to the find-missing-translations.js script
  • Implemented checkPackageNlsTranslations() function to check all package.nls.*.json files against the base package.nls.json
  • Updated the default area from "both" to "all" to include package-nls checks by default
  • Added proper error handling and reporting for package.nls files

Testing

The script was tested and successfully detected 2 missing translation keys across all 17 language-specific package.nls files:

  • commands.preventCompletionWithOpenTodos.description
  • settings.newTaskRequireTodos.description

Usage

# Check only package.nls files
node scripts/find-missing-translations.js --area=package-nls

# Check all areas (core, webview, and package-nls)
node scripts/find-missing-translations.js --area=all

# Check specific locale
node scripts/find-missing-translations.js --area=package-nls --locale=fr

This enhancement ensures that all translation files, including the VSCode package.nls files, are properly checked for missing translations.

Important

Add package.nls.json checking to find-missing-translations.js script, including new area option and error handling.

  • Behavior:
    • Added package-nls area option to find-missing-translations.js.
    • Implemented checkPackageNlsTranslations() to compare package.nls.*.json files against package.nls.json.
    • Default area changed from both to all to include package-nls by default.
    • Enhanced error handling for package.nls files.
  • Testing:
    • Detected 2 missing translation keys across 17 language-specific package.nls files.
  • Usage:
    • node scripts/find-missing-translations.js --area=package-nls to check only package.nls files.
    • node scripts/find-missing-translations.js --area=all to check all areas.
    • node scripts/find-missing-translations.js --area=package-nls --locale=fr to check a specific locale.

This description was created by Ellipsis for 496ce25. You can customize this summary. It will automatically update as commits are pushed.

@roomote roomote bot requested review from cte, jr and mrubens as code owners September 23, 2025 11:57
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. Enhancement New feature or request labels Sep 23, 2025
ellipsis-dev[bot]
ellipsis-dev bot reviewed Sep 23, 2025
View reviewed changes
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 23, 2025
roomote[bot]
roomote bot commented Sep 23, 2025
View reviewed changes
Copy link
Contributor Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing my own code is like grading my own homework - suspiciously perfect yet somehow still needs work.

@brunobergher
Copy link
Collaborator

brunobergher commented Sep 23, 2025

@roomote-agent implement the PR feedback.

@brunobergher brunobergher marked this pull request as draft September 23, 2025 13:10
@roomote
Copy link
Contributor Author

roomote bot commented Sep 23, 2025

Hi @brunobergher! I see the PR feedback and will implement the following improvements:

  1. Extract shared JSON parsing utility - Reduce code duplication between checkAreaTranslations() and checkPackageNlsTranslations()
  2. Add validation for flat JSON structure - Ensure package.nls files maintain the expected flat structure
  3. Add warning message - Inform users when package.nls.json is not found
  4. Performance optimization - Convert to async/await for parallel file reading

Working on these changes now!

@roomote
refactor: improve find-missing-translations script based on PR feedback
b59f20c 
- Extract shared JSON parsing utility to reduce code duplication
- Add validation for flat JSON structure in package.nls files
- Add warning message when package.nls.json is not found
- Convert to async/await for parallel file reading for better performance
- Improve error handling and consistency across all areas
@roomote roomote bot force-pushed the feat/check-package-nls-translations branch from 4960fe6 to b59f20c Compare September 23, 2025 13:20
@roomote
Copy link
Contributor Author

roomote bot commented Sep 23, 2025

Hi @brunobergher! I have successfully implemented all the PR feedback:

✅ Changes Implemented

  1. Extracted shared JSON parsing utility (parseJsonFile)

    • Eliminates code duplication between checkAreaTranslations() and checkPackageNlsTranslations()
    • Provides consistent error handling across all file reads

  2. Added validation for flat JSON structure

    • New validateFlatStructure() function ensures package.nls files maintain expected flat structure
    • Exits with error if nested objects are detected

  3. Added warning message for missing package.nls.json

    • Now displays a clear warning when the base file is not found
    • Gracefully skips package.nls checks instead of erroring out

  4. Converted to async/await for parallel file reading

    • All file operations now use async/await with Promise.all()
    • Significantly improves performance when checking multiple locales
    • Parallel processing of locale files and translation checks

📊 Test Results

The script is working correctly and detecting the 2 missing translation keys across all 17 language files:

  • commands.preventCompletionWithOpenTodos.description
  • settings.newTaskRequireTodos.description

The check-translations CI job is failing as expected due to these missing translations, which is the intended behavior to enforce translation completeness.

All other CI checks are passing successfully! 🎉

@brunobergher brunobergher marked this pull request as ready for review September 23, 2025 13:40
ellipsis-dev[bot]
ellipsis-dev bot reviewed Sep 23, 2025
View reviewed changes
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Sep 23, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 23, 2025
daniel-lxs
daniel-lxs approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Sep 23, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 23, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Sep 23, 2025
mini2s added a commit to zgsm-ai/costrict that referenced this pull request Sep 25, 2025
@mini2s @github-actions
@mrubens @roomote @roomote @daniel-lxs @cte @jr @DC-Dancao @wangxiaolong100 @PeterDaveHello @Thachnh @hannesrudolph @ellipsis-dev @CellenLee @NaccOll @HahaBill @ssweens @renovate @ItsOnlyBinary @brunobergher @mubeen-zulfiqar @dependabot @jdilla1277 @snova-jorgep @requesty-JohnCosta27
Roo to main (#482