improve i18n parser to support console-extensions.json #8363

christianvogt · 2021-03-10T22:39:54Z

Dynamic plugins contribute extensions through the console-extensions.json. To localize a string in json, the string must match the format /%.+%/.
eg.

{
  "foo": "%bar%"
}

This PR adds a custom lexer which parses *.json files and matches string values against the localization pattern.

Added unit test for custom JSON lexer.

Update the i18n-scripts/.eslintrc.js rules to reuse those defined in the console eslint plugin.

cc @rohitkrai03 @vojtechszocs @rebeccaalpert @spadgett

Before this change we would require a comment to pick up the localized string. Although there is no lexer currently for json files.

  [
    {
      "type": "AddAction",
      "flags": {
        "required": ["OPENSHIFT_HELM"]
      },
      "properties": {
        "id": "helm",
        "url": "/catalog?catalogType=HelmChart",
        // t('helm-plugin~Helm Chart')
        "label": "%helm-plugin~Helm Chart%",
        // t('helm-plugin~Browse the catalog to discover and install Helm Charts')
        "description": "%helm-plugin~Browse the catalog to discover and install Helm Charts%"
      }
    }
  ]

After this change, the lexer will pickup the string without the comment:

  [
    {
      "type": "AddAction",
      "flags": {
        "required": ["OPENSHIFT_HELM"]
      },
      "properties": {
        "id": "helm",
        "url": "/catalog?catalogType=HelmChart",
        "label": "%helm-plugin~Helm Chart%",
        "description": "%helm-plugin~Browse the catalog to discover and install Helm Charts%"
      }
    }
  ]

locale file: helm-plugin.json

{
  "Helm Chart": "Helm Chart",
  "Browse the catalog to discover and install Helm Charts": "Browse the catalog to discover and install Helm Charts"
}

rebeccaalpert

Looks good to me. Just one comment about a console.log that looked like it may not be intentional. Thanks for cleaning up the linter file and making some of the other scripts easier to read.

rebeccaalpert · 2021-03-11T14:11:16Z

frontend/i18n-scripts/lexers.js

+      JSON.parse(content, (key, value) => {
+        if (typeof value === 'string') {
+          const match = value.match(/%(.+)%/);
+          console.log('testing:', match[1]);


Was this console.log intentional?

Most likely it's just to test the lexer as it was developed, I think we can remove it now.

vojtechszocs · 2021-03-11T16:02:46Z

frontend/i18n-scripts/.eslintrc.js

-  globals: {
-    process: 'readonly',
+    'no-console': 'off',
+    // fs.promises requires a newer version of node however our compliance is set to node >=10


FYI once #7306 is merged, our compliance will be Node.js >= 14.

right, but not there yet :P

vojtechszocs · 2021-03-11T16:09:53Z

frontend/i18n-scripts/i18n-to-po.js

-      ),
-    );
+      )
+      .catch((e) => console.error(e, fileName));


Suggested change

.catch((e) => console.error(e, fileName));

.catch((e) => console.error(fileName, e));

We can print context-specific bits (like fileName) before printing error & its stack trace.

vojtechszocs · 2021-03-11T16:10:24Z

frontend/i18n-scripts/i18n-to-po.js

+          language,
+        ),
+      )
+      .catch((e) => console.error(e, fileName));


Suggested change

.catch((e) => console.error(e, fileName));

.catch((e) => console.error(fileName, e));

vojtechszocs · 2021-03-11T16:12:41Z

frontend/i18n-scripts/lexers.js

+    try {
+      JSON.parse(content, (key, value) => {
+        if (typeof value === 'string') {
+          const match = value.match(/%(.+)%/);


Suggested change

const match = value.match(/%(.+)%/);

const match = value.match(/^%(.+)%$/);

vojtechszocs · 2021-03-11T16:13:30Z

frontend/i18n-scripts/lexers.js

+      JSON.parse(content, (key, value) => {
+        if (typeof value === 'string') {
+          const match = value.match(/%(.+)%/);
+          console.log('testing:', match[1]);


Most likely it's just to test the lexer as it was developed, I think we can remove it now.

vojtechszocs · 2021-03-11T16:15:26Z

frontend/i18n-scripts/po-to-i18n.js

-  gettextToI18next(language, fs.readFileSync(fileName)).then(save(newFilePath));
+  gettextToI18next(language, fs.readFileSync(fileName))
+    .then(save(newFilePath))
+    .catch((e) => console.error(e, fileName));


Suggested change

.catch((e) => console.error(e, fileName));

.catch((e) => console.error(fileName, e));

vojtechszocs · 2021-03-11T16:16:07Z

frontend/i18n-scripts/set-english-defaults.js

-  });
+  fs.promises
+    .writeFile(fileName, JSON.stringify(updatedFile, null, 2))
+    .catch((e) => console.error(e));


Suggested change

.catch((e) => console.error(e));

.catch((e) => console.error(fileName, e));

christianvogt · 2021-03-11T17:07:47Z

@vojtechszocs thanks for the review. I've updated all the suggestions.

christianvogt · 2021-03-11T18:14:26Z

updated lexer parser to use comment-json to properly parse json files with comments as this is supported by dynamic plugin extensions files.

vojtechszocs · 2021-03-11T19:14:32Z

/lgtm

openshift-ci-robot · 2021-03-11T19:14:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, rebeccaalpert, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt,vojtechszocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rohitkrai03 · 2021-03-11T23:47:28Z

/retest

openshift-bot · 2021-03-12T03:34:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Mar 10, 2021

openshift-ci-robot requested review from rebeccaalpert and rhamilto March 10, 2021 22:40

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2021

christianvogt force-pushed the i18n-json branch from 4708778 to 5a574a4 Compare March 11, 2021 14:40

rebeccaalpert approved these changes Mar 11, 2021

View reviewed changes

vojtechszocs suggested changes Mar 11, 2021

View reviewed changes

christianvogt mentioned this pull request Mar 11, 2021

Migrate all static AddAction extensions to dynamic extensions #8371

Merged

4 tasks

update eslint rules for i18n-scripts

d56657e

christianvogt force-pushed the i18n-json branch from 5a574a4 to dd08bf8 Compare March 11, 2021 17:07

christianvogt force-pushed the i18n-json branch from adfc46b to f49bbef Compare March 11, 2021 18:13

custom i18n json lexer for pattern /^%.+%$/

3012123

christianvogt force-pushed the i18n-json branch from f49bbef to 3012123 Compare March 11, 2021 19:07

openshift-ci-robot added the component/sdk Related to console-plugin-sdk label Mar 11, 2021

openshift-ci-robot assigned vojtechszocs Mar 11, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

openshift-merge-robot merged commit 5f9508b into openshift:master Mar 12, 2021

spadgett added this to the v4.8 milestone Mar 18, 2021

christianvogt deleted the i18n-json branch June 2, 2021 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve i18n parser to support console-extensions.json #8363

improve i18n parser to support console-extensions.json #8363

christianvogt commented Mar 10, 2021 •

edited

rebeccaalpert left a comment

rebeccaalpert Mar 11, 2021

vojtechszocs Mar 11, 2021

christianvogt Mar 11, 2021

vojtechszocs Mar 11, 2021

christianvogt Mar 11, 2021

vojtechszocs Mar 11, 2021

vojtechszocs Mar 11, 2021

vojtechszocs Mar 11, 2021

vojtechszocs Mar 11, 2021

vojtechszocs Mar 11, 2021

vojtechszocs Mar 11, 2021

christianvogt commented Mar 11, 2021

christianvogt commented Mar 11, 2021

vojtechszocs commented Mar 11, 2021

openshift-ci-robot commented Mar 11, 2021

rohitkrai03 commented Mar 11, 2021

openshift-bot commented Mar 12, 2021

	.catch((e) => console.error(e, fileName));
	.catch((e) => console.error(fileName, e));

	const match = value.match(/%(.+)%/);
	const match = value.match(/^%(.+)%$/);

	.catch((e) => console.error(e));
	.catch((e) => console.error(fileName, e));

improve i18n parser to support console-extensions.json #8363

improve i18n parser to support console-extensions.json #8363

Conversation

christianvogt commented Mar 10, 2021 • edited

rebeccaalpert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Mar 11, 2021

christianvogt commented Mar 11, 2021

vojtechszocs commented Mar 11, 2021

openshift-ci-robot commented Mar 11, 2021

rohitkrai03 commented Mar 11, 2021

openshift-bot commented Mar 12, 2021

christianvogt commented Mar 10, 2021 •

edited