-
Notifications
You must be signed in to change notification settings - Fork 919
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
Split .i18nrc across src
, packages
, and examples
#8414
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8414 +/- ##
=======================================
Coverage 60.92% 60.92%
=======================================
Files 3750 3751 +1
Lines 89104 89149 +45
Branches 13926 13935 +9
=======================================
+ Hits 54284 54318 +34
- Misses 31443 31451 +8
- Partials 3377 3380 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5e94c56
to
d0dbf5c
Compare
Also: * Force validation of i18n even when no translations are available * Allow including translations in built artifacts Signed-off-by: Miki <[email protected]>
d0dbf5c
to
ac7150a
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.
If this file is here, its likely because of our plugin scaffolding code. Lets modify it too to support generating the i18nrc code?
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.
OpenSearch-Dashboards/packages/osd-plugin-generator/src/render_template.ts
Lines 104 to 107 in d7004dc
excludeFiles( | |
([] as string[]).concat( | |
answers.ui ? [] : 'public/**/*', | |
answers.ui && !answers.internal ? [] : ['translations/**/*', 'i18nrc.json'], |
While generating a plugin, this piece decides if the i18n manifest and sample translation should be included or not.
- If it is not a UI plugin, no i18n files will be included.
- if a plugin is internal, no i18n files will be included.
data_explorer
was probably generated as an external plugin and then they changes their minds.
Prior to this PR, everything in OSD shared one manifest and one translations folder. This PR splits them into 3 main groups of stuff under src
, packages
, and examples
.
HOWEVER, my plan is to have i18n look through the 3 root folders recursively so src/core
, and each internal plugin, package, and example can own its translations. I want to have some form of inheritance so common terms like Save
and Save as
need not be translated several times for each namespace.
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.
Makes sense :)
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 i create a new example or core or external plugin, how will this be updated? I think one of the reasons for keeping the i18nrc file in each plugin migt have been to make scaffolding easier (just speculation)
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.
Each external plugin will continue to own its own i18n manifest and translations. Each internal plugin, package, or example will eventually have their own manifest too, but with this PR, each will use the translations from the src
, packages
, or examples
. Prior to this PR, they all had to pick from one central place.
There is logic which I think "imports" translations files and distributes them to the folder of each manifest based on the namesspaces defined in them. All of these will tie into each other when and if I get to next phase.
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.
Can you document this in a rewadme as a part of the change so that:
- Its clear to someone how translations works in OSD after this change.
- If you dont get to the next phase yourself, you still have it written down somewhere that someone can take your vision and execute it
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.
A readme would be helpful - I really don't know how this works so harder to review
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.
We have https://github.com/opensearch-project/dashboards-i18n/blob/main/DEVELOPER_GUIDE.md and https://github.com/opensearch-project/OpenSearch-Dashboards/tree/main/packages/osd-i18n. Some day, I will merge those into one and add my future vision into it.
if (options.withTranslations) { | ||
// control w/ --with-translations | ||
await run(Tasks.CopyTranslations); | ||
} |
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.
Nice!
"embeddableExamples": "embeddable_examples", | ||
"expressionsExample": "expressions_example", | ||
"multipleDataSourceExample": "multiple_data_source_examples", | ||
"searchExamples": "search_examples", |
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.
Is the difference in total number of paths across these .i18nrc.json files due to adding some that were not added to the original ones?
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.
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.
A readme would be helpful - I really don't know how this works so harder to review
Also: * Force validation of i18n even when no translations are available * Allow including translations in built artifacts Signed-off-by: Miki <[email protected]> (cherry picked from commit f3f007a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Also: * Force validation of i18n even when no translations are available * Allow including translations in built artifacts (cherry picked from commit f3f007a) Signed-off-by: Miki <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Split .i18nrc across
src
,packages
, andexamples
Note: This is the first step in recursively looking for translations. These 3 will be the roots where i18n will recursively search.
Also:
Changelog
src
,packages
, andexamples
Check List
yarn test:jest
yarn test:jest_integration