Skip to content
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

[i18n] .i18nrc file as the source of truth and enhance tooling #39774

Merged
merged 26 commits into from
Jul 21, 2019

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Jun 27, 2019

Dev Docs

Currently there is a disconnect disconnect between i18n tooling (i18nrc.json) and top-level/translations convention used by the kibana i18n service. This change attempts to fix it, tidying up the code a little, and enhancing the i18n tooling as a result of this change.

Use .i18nrc.json as the source of truth

Currently we assume that internal and external plugins to follow this convention of putting translations following this pattern **/*/translations/${locale}.json.

We also have to keep the .i18nrc in sync with the location of each of these translation files.

This change makes the i18n service look into top level .i18nrc.json files and parses the translations instead of traversing the source code directories to look for translation files.

This change discards the translation paths assumptions and allows developers to explicitly specify translation paths inside .i18nrc file. Previously the translations paths in the .i18nrc was only used for the scripts/i18n_check.js tooling.

This change also allows for a better third party plugin development experience as developers can fully control their own translation paths using the .i18nrc.json file.

Enhance i18n tooling (namespaces prefix checking, better logging and error handling for parsing .i18nrc files)

Introducing top level .i18nrc files for plugins allows adding a namespaces prefix check.
This PR adds a prefix check that all x-pack plugin namespaces (label ids) start with xpack.*

Introducing top level .i18nrc files for plugins allows for a better experience in merging configs. This PR adds a more verbose logging while merging configs and better error handling.

render1561625817600

@Bamieh Bamieh added Project:i18n release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.3.0 labels Jun 27, 2019
@Bamieh Bamieh requested a review from a team as a code owner June 27, 2019 08:59
@Bamieh Bamieh requested review from azasypkin and a team June 27, 2019 09:00
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Contributor

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

I'm going to defer to @azasypkin here, since I'm not super familiar with the i18n stuff, but I do remember us pretty explicitly wanting the translations to not require a config file to find and instead that we wanted to use consistent paths. Not saying we can't change course, but I want to make sure it's well considered and I don't think I have enough context to do so.

@Bamieh
Copy link
Member Author

Bamieh commented Jun 27, 2019

@spalger Sounds good. I did had a chat with Oleg while working on this, lets wait for his feedback.

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

Looks like you left some commented out code in, but otherwise LGTM.

@@ -324,3 +324,8 @@ export class ErrorReporter {
);
}
}

// export function arrayify<Subj = any>(subj: Subj | Subj[]): Subj[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there commented out code here? Did you start typescripting this file and then stop?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it for whenever we want to mvoe this file to TS

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bamieh
Copy link
Member Author

Bamieh commented Jun 27, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bamieh Bamieh requested a review from eliperelman July 3, 2019 12:41
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger added the v7.4.0 label Jul 3, 2019
@Bamieh Bamieh removed the v7.3.0 label Jul 3, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bamieh Bamieh requested a review from spalger July 10, 2019 12:55
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Couple nits, but LGTM (code review only)

src/dev/i18n/config.ts Show resolved Hide resolved
src/dev/i18n/tasks/check_configs.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bamieh Bamieh merged commit 81153c2 into elastic:master Jul 21, 2019
@Bamieh Bamieh deleted the i18n/packages-translations branch July 21, 2019 12:06
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 22, 2019
…b-panel-for-stopping-jobs

* 'master' of github.com:elastic/kibana: (58 commits)
  [DOCS] Timelion cleanup (elastic#41381)
  [Docs] Add simple phrase highlighting to Logs UI (elastic#41610)
  [Maps] Rename modules for clarity (elastic#41608)
  [Monitoring] Metricbeat migration net new user experience (elastic#39832)
  [Maps] Only color legend icon with dynamic color when dynamic config is complete (elastic#41607)
  [TSVB] [Markdown] markdown section do not render after change data parameter (elastic#41576)
  [Vega] (Step 2) Shim new platform - renaming vega -> vis_type_vega (elastic#41565)
  update dark mode tsvb test (elastic#41618)
  [i18n] .i18nrc file as the source of truth and enhance tooling (elastic#39774)
  Reactify Top Nav Menu (kbn_top_nav) (elastic#40262)
  fix(code/frontend): should update search results if search options change (elastic#41232)
  Use kibana-ci-proxy-cache for chrome and gecko drivers (elastic#41581)
  [SIEM] Fix draggables to work with escapeId for the ML severity column (elastic#41621)
  [Canvas] Updates esdocs default count to 1000 (elastic#41604)
  [Uptime] Fix duration chart for Safari (elastic#41619)
  [Canvas] Restores "Today" as a quick time range in time filter (elastic#41528)
  docs: lowercase app (elastic#41612)
  [Code] Update git repository update frequency (elastic#41541)
  Remove language=json on code blocks due to performance hit (elastic#41540)
  [DOCS] Update anchors and links for Elasticserach API relocation. (elastic#41372)
  ...
@stacey-gammon
Copy link
Contributor

@Bamieh - has this been backported to 7.4?

@Bamieh
Copy link
Member Author

Bamieh commented Aug 14, 2019

@stacey-gammon oh! PR submitted

stacey-gammon pushed a commit that referenced this pull request Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport pending Project:i18n release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants