-
Notifications
You must be signed in to change notification settings - Fork 171
[GSOC 2025]: Github action for translation tracker #895
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Updated translation-sync workflow for examples instead of reference - Added Week 3 test change to Shape Primitives description - Should trigger automated issue creation for outdated translations
- Added workflow_dispatch with scan_all and create_issues options - Can now manually trigger action from GitHub UI - Supports both normal and full-scan modes
- Modified description.mdx to test translation tracker - Should now detect 1 English example file change
- test-local.js: Only shows changed files from predefined test files - index.js: Production mode with minimal console output - Removed verbose logging and repository exploration - Clear distinction: test mode vs production mode with GitHub token
- SCAN_ALL=true environment variable to check all 61 example files - Simplified GitHub Actions workflow - Default: checks only changed files - Manual trigger: option to scan all files
Hi @Divyansh013 , this is looking great! Based on checking the current issue created by this (Divyansh013#11) I have a couple of minor suggestions:
|
? 'git diff --name-only HEAD~1 HEAD' | ||
: 'git diff --name-only HEAD~1 HEAD'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two options are actually the same. Maybe typo here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed. What was originally intended here:
'git diff --name-only origin/main...HEAD' // Compare with base branch for PRs
: 'git diff --name-only HEAD~1 HEAD'; // and with last commir for pushes
thoughts about this?
|
||
return null; | ||
} catch (error) { | ||
// Fallback to main branch if current branch fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering what will happen if the main branch also fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case we can log out the error ? or is there something I am missing out?
try { | ||
return fs.statSync(filePath).mtime; | ||
} catch (error) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add log here might be helpful for debugging.
* Format the issue body with helpful information | ||
*/ | ||
formatIssueBody(englishFile, language, commitInfo) { | ||
const translationPath = englishFile.replace('/en/', `/${language}/`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the directory structure changes or there are multiple occurrences of ·/en/· in the path (correct me if there are any situations that I didn't catch), this could lead to errors or unintended behavior.
@ksen0 requesting your review here |
'zh-Hans': { | ||
title: '此翻译可能已过期', | ||
message: '与英文版本相比,此页面不是最新的。', | ||
viewEnglish: '查看英文页面', | ||
contribute: '参与翻译', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Chinese translation here is pretty good. I will take a look of the remaining GitHub Actions related stuff when I have time.
contribute: 'Contribuir a la traducción', | ||
}, | ||
'ko': { | ||
title: '이 번역은 오래되었을 수 있습니다', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hana-cho! When you have time, another set of Korean translations for your review. Thanks so much!
contribute: 'अनुवाद में योगदान दें', | ||
}, | ||
'es': { | ||
title: 'Esta traducción podría estar desactualizada', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marioguzzzman! If/when you have time, could you check these Spanish translations please? Thanks so much!
let englishUrl = Astro.url.pathname; | ||
if (currentLocale !== 'en') { | ||
// Replace /{locale}/ with / to get English URL | ||
englishUrl = englishUrl.replace(`/${currentLocale}/`, '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question here: have you tried using zh-Hans
? It is a different use case from es
, hi
and ko
. I’m wondering if the replace operation will succeed.
PS: Please check all places with currentLocale
and replace()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes have checked zh-Hans and it is working in expected way. I will check in all places
- Links to files and comparisons | ||
- Action checklist for translators | ||
- Proper labels (`needs translation`, `lang-es`, etc.) | ||
4. **Generates** manifest file for website banner system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the README updates! As one last thing, please include here or ~line 112 a short explanation/example of the translation-status
format.
const englishCommit = commitInfo.english; | ||
const translationCommit = commitInfo.translation; | ||
|
||
return `## 🌍 Translation Update Needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a future improvement, this could be pulled out as a template file separate form the .js script.
|
||
on: | ||
push: | ||
branches: [main, week2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great and nearly ready for further testing. If we're now in step 1 of the below iterative deployment process, then to get to step 2, I think this should listen for push on branch only for now
- Finalize this PR tested on your fork, document any further improvements, etc.
- create a new PR that contains only the action. This PR would target the main branch, but the action would listen for translation changes on the staging branch.
- create a PR that targets the staging branch to add the UI. This will allow fine-tuning the tool.
- Lastly, to make it go live, the process would be to marge the UI PR into main, and then change the action to listen on main rather than branch
/** | ||
* Scan all English example files (for manual scanning) | ||
*/ | ||
function getAllEnglishExampleFiles() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest next step to improve here would be to generalize this code so it could work with not only "examples/" but also tutorials/
and text-detail/
(still not including reference) - I've tried to add comments where applicable!
filesToCheck = getChangedFiles(); | ||
} else { | ||
console.log('📊 Scanning all English example files...'); | ||
filesToCheck = getAllEnglishExampleFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible usage: getAllExampleFiles(currentDirectory)
(where outside of the loop, currentDirectory
is looping through, e .g., ["examples"]
). To keep the issue volume down during testing, I still think should should only focus on one of the 3 directories (examples), but it should be easily extensible to include others, without refactor. Locally, would alos be great to test on the other 2 directories to see what edge-cases may come up.
examples, | ||
}; | ||
fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2), 'utf8'); | ||
console.log(`\n🗂️ Wrote translation manifest: ${manifestPath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include current directory in output
{ | ||
"name": "p5js-translation-tracker", | ||
"version": "0.2.0", | ||
"description": "GitHub Action to track translation status for p5.js examples and documentation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"GitHub Action to track translation status for p5.js examples and documentation" -> "GitHub Action to track translation status for p5.js documentation"
on: | ||
push: | ||
branches: [main, week2] | ||
paths: ['src/content/examples/en/**'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already looks generalized, which is great
|
||
## Features | ||
|
||
- Detects outdated/missing translations using Git commit comparison |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor: add a note here please that it currently focuses on Examples content
console.log('DEBUG: Current locale:', currentLocale); | ||
|
||
try { | ||
const manifestPath = path.join(process.cwd(), "public", "translation-status", "examples.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this stage, it makes sense to keep this here, but in the future (or now, optionally, if you prefer) also generalize it as a component usable in other layouts.
Uh oh!
There was an error while loading. Please reload this page.