i18n: added linter for common issues #23394
Conversation
WalkthroughThis change refactors the i18n (internationalization) linting and ignore management workflow. The Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between bcfe4bb19854e6a69a21c22a0cddb3eaa823c0b2 and 5a7d496. 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
vikaspotluri123
force-pushed
the
lint-i18n
branch
from
56ada66 to
ac3f3d8
Compare
There was a problem hiding this comment.
Thanks for your work on this! If I'd realized you were going to follow along behind me and do all of it better, I'd have worked on something different yesterday. laughing
I left a few comments. I'm going to also manually test some common scenarios to confirm that there aren't any surprises. :) Will update when I do.
ghost/i18n/test/i18n-ignores.json
Outdated
| { | ||
| "file": "da/portal.json", | ||
| "key": "Try free for {amount} days, then {originalPrice}.", | ||
| "comment": "This translation CANNOT work. Please repair." | ||
| "comment": "FIXME: This error was automatically ignored. Please update the translation or this comment." |
There was a problem hiding this comment.
I suggest retaining something similar to the wording I had for the added/undefined variables. There's no way that the translation will work for these addedVariable/no-undefined-variables problems. They're undefined by definition, so updating the comment doesn't make sense here. They need to be fixed in the translation.
No objections for the unused/missing variables.
There was a problem hiding this comment.
Will follow up once we close on #23394 (comment)
There was a problem hiding this comment.
I think this has been addressed
| ignores[rule].push({ | ||
| file, | ||
| key, | ||
| comment: 'FIXME: This error was automatically ignored. Please update the translation or this comment.' |
There was a problem hiding this comment.
I think a different comment for the rule type would be helpful for translators, who are often pretty new to things. Added variables are never fixable with a comment and should be fixed in the ns.json file. Missing variables could be intentional, but there's no functional reason to have an added variable. They are typos, copy-paste errors, or misunderstandings of how the t function works.
There was a problem hiding this comment.
IMO only you should be running --unsafe-ignore-all 😛
Translators should manually update the file if they add an ignore (since it's supposed to be very rare), and --fix will remove unused ignores.
There was a problem hiding this comment.
Right, agreed, and fortunately our average translator doesn't even have a dev install setup and isn't going to run anything.
I'm pretty much expecting not to run it either, BUT, for that initial commit, I'd like the FIXME comments to reflect that updating the comment is /not/ a fix for the made-up variable names, while it might be reasonable for the variables that don't appear in the translations.
In other words, please don't tell translators that updating the comment is an option for cases where they have made up a variable or have a typo in the variable name. It isn't.
There was a problem hiding this comment.
Ahh, I should probably fix the current comments like they're telling me to 😝
Are you okay with this copy for future runs? I was planning on having a single default string, but if you want one per rule that's pretty easy to do.
It can also say "[...], or explain why it's ignored (if applicable)"
There was a problem hiding this comment.
Since I don't plan to merge anything with an added variable in the value (ugh), I don't care too much what the --unsafe-ignore-all behavior is (unless you're overwriting existing comments, which I don't think you are). But I do want the initial commit to not encourage translators to adjust comments for added/incorrect variables in values. :)
|
|
||
| // Report key parsing errors only for English translations. | ||
| // I18next is responsible for keeping all other locales in sync. | ||
| if (reportInvalidTranslations && context.file?.locale === 'en') { |
There was a problem hiding this comment.
Flagging this to look at further - are we sure that there won't be errors of this type in the other files once the translators have touched them? I've seen a lot of weirdness...
There was a problem hiding this comment.
Running yarn translate should cover it, we can configure it to be a gating check in CI (run it with --fail-on-update)
vikaspotluri123
force-pushed
the
lint-i18n
branch
from
ac3f3d8 to
8f3b361
Compare
vikaspotluri123
force-pushed
the
lint-i18n
branch
from
8f3b361 to
cc9d967
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ghost/i18n/test/i18n-ignores.json (1)
1-173: Rule naming and comment updates properly address reviewer feedback.The ESLint-style rule naming and standardized comments effectively implement the feedback from previous reviews. The distinction between undefined variables (which must be fixed) and unused variables (which might be intentional) is now clear in the comment text.
ghost/i18n/test/i18n.lint.js (1)
392-393: Default ignore comment could be more specific per rule type.Based on past review feedback, the comment text should distinguish between rule types. Consider using different default comments for
no-undefined-variablesvsno-unused-variablesrules.function ignoreTranslationError(ignores, rule, file, key) { ignores[rule] ??= []; + const commentMap = { + 'ghost/i18n/no-undefined-variables': 'FIXME: This translation doesn\'t work and needs to be updated', + 'ghost/i18n/no-unused-variables': 'FIXME: This error was automatically ignored. Please update the translation, or explain why a variable is ignored' + }; ignores[rule].push({ file, key, - comment: 'FIXME: This error was automatically ignored. Please update the translation or this comment.' + comment: commentMap[rule] || 'FIXME: This error was automatically ignored. Please update the translation or this comment.' }); }
🧹 Nitpick comments (1)
ghost/i18n/test/i18n.lint.js (1)
48-87: Consider robustness of probabilistic position mapping.The
_analyzedSource()method assumes one key/value pair per line, which may not hold for all JSON formatting styles (e.g., minified JSON or different prettier configurations). This could lead to incorrect line/column reporting.Consider using a proper JSON parser with position tracking or add validation to ensure the JSON follows the expected format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a9cd809 and bcfe4bb19854e6a69a21c22a0cddb3eaa823c0b2.
📒 Files selected for processing (5)
ghost/i18n/package.json(1 hunks)ghost/i18n/test/i18n-ignores.json(1 hunks)ghost/i18n/test/i18n.lint.js(1 hunks)ghost/i18n/test/i18n.test.js(6 hunks)ghost/i18n/test/remove-unneeded-ignores.js(0 hunks)
💤 Files with no reviewable changes (1)
- ghost/i18n/test/remove-unneeded-ignores.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 22.13.1, mysql8)
🔇 Additional comments (4)
ghost/i18n/test/i18n.test.js (1)
1-168: LGTM! Test removal aligns with the new linting approach.The removal of the ignore validation and translation variable tests is appropriate since this functionality is now handled by the new
i18n.lint.jsscript. The remaining tests focus on core i18n functionality, which is the correct separation of concerns.ghost/i18n/package.json (1)
12-16: Script updates correctly integrate the new linting workflow.The removal of
test:update-ignoresand addition oflint:translationsproperly reflects the transition from test-based validation to lint-based validation. The new lint script is correctly integrated into the mainlintcommand.ghost/i18n/test/i18n.lint.js (2)
371-377: Good future-proofing with the fs.glob comment.The TODO comment about using
fs.globwhen Node 22 is available shows good awareness of upcoming Node.js features. The current glob implementation works well as a temporary solution.
259-259:✅ Verification successful
Verify Node.js version compatibility for
structuredClone.The
structuredClonefunction requires Node.js 17 or higher. Ensure this meets your project's minimum Node.js version requirements.
🏁 Script executed:
#!/bin/bash # Check the minimum Node.js version requirement in the project fd -t f "package.json" -x grep -l "engines" {} \; | head -5 | xargs grep -A2 -B2 "engines" # Check if there's a .nvmrc or similar file specifying Node version fd -t f "\.nvmrc|\.node-version" | head -5 | xargs cat 2>/dev/nullLength of output: 1487
structuredClone compatibility confirmed
The project’s
package.json(in bothghost/coreandghost/admin) specifies Node^18.12.1 || ^20.11.1 || ^22.13.1, which exceeds the Node 17+ requirement for usingstructuredClone. No changes are necessary.
ref TryGhost#23385 - We currently detect issues with translations using Mocha, but this is pretty limiting - Created a custom iterator that generates and prints eslint-style messages. It: - Reports mismatched brackets (no change) - Reports undefined variables (no change) - Reports unused ignore rules - Can remove unused ignores - Can unsafely ignore all errors - Provides all errors at once - Includes positions in file (mostly accurately)
- superseded by the new linter
vikaspotluri123
force-pushed
the
lint-i18n
branch
from
bcfe4bb to
5a7d496
Compare
ref #23385
merge strategy: squash
We currently detect issues with translations using Mocha, but this is pretty limiting
Created a custom iterator that generates and prints eslint-style messages. It:
Please check your PR against these items:
I've read and followed the Contributor Guide
I've explained my change
I've written an automated test to prove my change works
We appreciate your contribution! 🙏