-
Notifications
You must be signed in to change notification settings - Fork 204
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
Rewrite the i18n scripts #5177
base: main
Are you sure you want to change the base?
Rewrite the i18n scripts #5177
Conversation
415f12a
to
91e3150
Compare
Latest k6 run output1
Footnotes
|
7cde493
to
e528812
Compare
Full-stack documentation: https://docs.openverse.org/_preview/5177 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
2d8ae64
to
362799e
Compare
1eed35d
to
6e19cc4
Compare
362799e
to
b0b0537
Compare
6e19cc4
to
5afc4b5
Compare
b0b0537
to
d2c3fea
Compare
1c42674
to
4c5854e
Compare
- Convert CJS to MJS - Use flat keys and simplify json handling - Update ESLint rule to allow for dot in keys
4c5854e
to
86e1b36
Compare
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 cool! Thanks for the ping on the issues closing. Did a quick little review and have some simple suggestions. What a nice clean up and reorganisation 🙂
@@ -78,7 +75,7 @@ processed and loaded into Nuxt via the Nuxt i18n module. | |||
## Test locales | |||
|
|||
Three locales are maintained in the code base (as opposed to downloaded from | |||
Glotpress) for use in testing, alongside the default English locale. These | |||
GlotPress) for use in testing, alongside the default English locale. These |
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.
tip: You can add GlotPress to the Vale TermCasing.yaml
so these errors are found automatically in the future.
export type CasingKind = "camelCaseWithDot" | "snake_case_with_dot" | ||
|
||
export const allowedCaseOptions: CasingKind[] = [ | ||
"camelCaseWithDot", | ||
"snake_case_with_dot", | ||
] |
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.
Just a tip, if you want to avoid duplicating the names in this definition, you can derive CasingKind
from allowedCaseOptions
:
export type CasingKind = "camelCaseWithDot" | "snake_case_with_dot" | |
export const allowedCaseOptions: CasingKind[] = [ | |
"camelCaseWithDot", | |
"snake_case_with_dot", | |
] | |
export const allowedCaseOptions = [ | |
"camelCaseWithDot", | |
"snake_case_with_dot", | |
] as const | |
export type CasingKind = typeof allowedCaseOptions[number] |
Alternatively, you can derive both from the checkersMap
:
const checkersMap = {
camelCaseWithDot: isCamelCase,
snake_case_with_dot: isSnakeCase,
} as const
export type CasingKind = keyof typeof checkersMap
export const allowedCaseOptions: CasingKind[] = Object.keys(checkersMap)
function hasSymbols(str: string) { | ||
return /[\u0021-\u0023\u0025-\u002c./\u003a-\u0040\u005b-\u005e`\u007b-\u007d]/u.test( | ||
str | ||
) // without " ", "$", "-" and "_" |
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.
Are these ranges inclusive of the bounds? When I check https://symbl.cc/en/unicode-table/, 0025 is the space character. Should the the second range be updated to 0026?
Also: I'm not sure whether the inclusion of .
, /
and `
separate from the unicode ranges is intentional, but if it is, it might be helpful to move them to the start or end of the bracket group so they do not get lost.
return /[\u0021-\u0023\u0025-\u002c./\u003a-\u0040\u005b-\u005e`\u007b-\u007d]/u.test( | ||
str | ||
) // without " ", "$", "-" and "_" |
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.
Because this rule is only used for locale json files, it might be easier to understand this particular check by inverting it to check the characters that are allowed in a locale key. Would that be, [a-zA-Z._]
, for example? If this rule is only used for locale key strings, and that would allow simplifying aspects of it by making it more exact, then it's worth doing. This regex is a little obscure as it is 😅
In that case, it would also be good to change the name of the rule to @openverse/locale-key-name-casing
.
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 copied the code as-is from the key name casing rule, only adding the .
:) Great suggestions, thank you!
/** | ||
* Return case checker ('camelCaseWithDot', 'snake_case_with_dot') | ||
*/ | ||
export function getChecker(name: "camelCaseWithDot"): (str: string) => boolean { |
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.
export function getChecker(name: "camelCaseWithDot"): (str: string) => boolean { | |
export function getChecker(name: CasingKind): (str: string) => boolean { |
Fixes
Fixes #4979 by @sarayourfriend
Fixes #629 by @obulat
Fixes #5059 by @obulat
Related to #592 by @sarayourfriend
Description
This PR re-writes the i18n locale metadata and translation files generation.
Folder structure
The recent update in the Nuxt i18n has introduced a new top-level i18n folder. Now all i18n-related files are located there:
i18n/data
folder contains the source-of-truth originalen.json5
file. The locale metadata (valid-locales.json
anduntranslated-locales.json
) are saved there as well.i18n/locales
folder is where all downloaded translations are saved.i18n/scripts
folder contains the scripts that download locale metadata, translations, converten.json5
to.pot
file that is used for translations on https://translate.wordpress.orgCode modernization
Now that it's easy to run modules in Node, the scripts can be converted to
.mjs
and useimport
instead ofrequire
. This PR contains the conversion, as well as migration fromaxios
to nativefetch
.Flattening of the keys
To simplify finding static and dynamic keys in the source code, as well as the conversion from and to
.pot
files, the keys of the translation strings have been flattened. So now,en.json5
is a flat JSON file, where all keys are top-level. This required the update to the ESLint rule for the json5 keys.Description of changed files (what to review)
The main entry point for the i18n scripts is now
i18n/scripts/setup.mjs
. This file reads the CLI options, and creates the necessary i18n files. All modes (English-only, production - downloading all locale metadata and translations, testing - copying of the local files from the test folder) are now combined in a single workflow.The order of the steps for production was changed because the
translated
property for the locale metadata is now generated based on the number of keys in the locale's json file, so we need to have the translations before creating the list of available locales.Previously, the steps were:
translated
property to the locales from step 1.json
files into nested json.valid-locales.json
file, and the locales without translations toinvalid-locales.json
.In this PR, step 2 was eliminated, and now the process is:
valid-locales.json
file, and the locales without translations toinvalid-locales.json
.The code for the new step 1 is located in
i18n/scripts/translations.mjs
, for the new step 2 and 3 - ini18n/scripts/metadata.mjs
.The code for generating the pot file was also simplified now that we don't need to handle nested json, and is now located in
i18n/scripts/generate-pot.mjs
.All of the paths for files are now located in a single file,
i18n/scripts/paths.mjs
, so it will be easier to safely move a file and update its paths in a single place.The i18n check script was removed from
frontend/package.json
because it wasn't working, and the library doesn't seem to be maintained.Testing Instructions
Run the i18n scripts, deleting the locale data between the runs to ensure that existing data does not interfere.
i18n
, but has more verbose logging, including the logging of keys/placeholders that have been modified to fix one of the linked GitHub issuesen.json
file and an emptyvalid-locales.json
. This can be used for quickly running local environment, including with the--watch
flag (that watches theen.json5
file and updatesen.json
when the original file changes)test/locales
into correct foldersvalid-locales.json
file based on existing translation files, without downloading translations from GlotPressen.json5
file topot
file. The generated file should be the same as the one generated onmain
.Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin