Localize using vscode-nls #10361

sadasant · 2022-06-08T03:46:44Z

This PR adds vscode-nls as a dependency, following the guidelines to make use of this tool to bring the standard VS Code localization to the vscode-jupyter extension.

For more information:

IMPORTANT CONSIDERATIONS:
Given that vscode-nls does not yet support the web environment, I haven't removed the existing localization approach, I have slightly changed it to support vscode-nls's API. The localize function will be using vscode-nls on Node.js, but on the web the custom approach we have been using so far is kept functionally intact.

Once vscode-nls supports the web, I will be able to remove the non-standard localization tool we currently have. I have made an issue to follow up: #10367

If this PR is merged:
Fixes #9676

sadasant · 2022-06-08T03:48:16Z

src/platform/common/utils/localize.ts

@@ -77,25 +80,28 @@ export namespace GitHubIssue {
        'Please provide a descriptive title summarizing your issue.'
    );
    export const titlePlaceholder = localize(
-        'GitHubIssue.titlePlaceholder',
+        { key: 'GitHubIssue.titlePlaceholder', comment: ['{Locked="kernel"}'] },


I am assuming we want to keep kernel as is and not change it from language to language.

sadasant · 2022-06-08T03:48:27Z

src/platform/common/utils/localize.ts

        'E.g.: Unable to start a local kernel session'
    );
    export const pleaseFillThisOut = localize(
        'GitHubIssue.pleaseFillThisOut',
        'Please fill this section out and delete this comment before submitting an issue!'
    );
    export const missingFields = localize(
-        'GitHubIssue.missingFields',
+        { key: 'GitHubIssue.missingFields', comment: ['{Locked="Issue"}'] },


I am assuming we want to keep the uppercase Issue as is and not change it from language to language.

sadasant · 2022-06-08T03:48:46Z

src/platform/common/utils/localize.ts

 }

 export namespace Logging {
-    export const currentWorkingDirectory = localize('Logging.CurrentWorkingDirectory', 'cwd:');
+    export const currentWorkingDirectory = () => 'cwd:';


I am assuming cwd: never changes.

sadasant · 2022-06-08T03:49:04Z

src/platform/common/utils/localize.ts

-        'DataScienceRendererExtension.installingExtension',
+        {
+            key: 'DataScienceRendererExtension.installingExtension',
+            comment: ['{Locked="Notebook Renderers"}']


I am assuming we want to keep Notebook Renderers as is and not change it from language to language.

sadasant · 2022-06-08T03:49:26Z

src/platform/common/utils/localize.ts

@@ -162,7 +183,7 @@ export namespace ExtensionSurveyBanner {
 export namespace DataScience {
    export const installingModule = localize('products.installingModule', 'Installing {0}');
    export const warnWhenSelectingKernelWithUnSupportedPythonVersion = localize(
-        'DataScience.warnWhenSelectingKernelWithUnSupportedPythonVersion',
+        { key: 'DataScience.warnWhenSelectingKernelWithUnSupportedPythonVersion', comment: ['{Locked="kernel"}'] },
        'The version of Python associated with the selected kernel is no longer supported. Please consider selecting a different kernel.'


I am assuming we want to keep kernel as is and not change it from language to language.

src/platform/common/utils/localize.ts

sadasant · 2022-06-08T03:50:17Z

src/platform/common/utils/localize.ts

-        'DataScience.checkingIfImportIsSupported',
-        'Checking if import is supported'
+        { key: 'DataScience.checkingIfImportIsSupported', comment: ['{Locked="import"}'] },
+        'Checking if "import" is supported'


I am assuming we want to keep import as is and not change it from language to language.

sadasant · 2022-06-08T03:51:11Z

src/platform/common/utils/localize.ts

        'Validating kernel dependencies'
    );
    export const performingExport = localize('DataScience.performingExport', 'Performing Export');
    export const convertingToPDF = localize('DataScience.convertingToPDF', 'Converting to PDF');
    export const exportNotebookToPython = localize(
-        'DataScience.exportNotebookToPython',
+        { key: 'DataScience.exportNotebookToPython', comment: ['{Locked="Notebook"}'] },


I am assuming we want to keep uppercase Notebook as is and not change it from language to language.

However, I am also assuming that sometimes it's ok to translate notebook, since it might help with context.

Let me know if I should keep all of the notebook instances as Locked.

sadasant · 2022-06-08T03:51:45Z

src/platform/common/utils/localize.ts

        'Raw kernel process exited before connecting.'
    );
    export const unknownMimeTypeFormat = localize(
-        'DataScience.unknownMimeTypeFormat',
+        { key: 'DataScience.unknownMimeTypeFormat', comment: ['{Locked="Mime type"}'] },
        'Mime type {0} is not currently supported'


I would not assume Mime type needs to be translated.

sadasant · 2022-06-08T03:52:02Z

src/platform/common/utils/localize.ts

@@ -362,39 +398,48 @@ export namespace DataScience {
    export const jupyterSelfCertClose = localize('DataScience.jupyterSelfCertClose', 'No, close the connection');
    export const pythonInteractiveHelpLink = localize(
        'DataScience.pythonInteractiveHelpLink',
-        'See [https://aka.ms/pyaiinstall] for help on installing jupyter.'
+        'See <https://aka.ms/pyaiinstall> for help on installing jupyter.'


The older one seemed to be prone to bugs, in my opinion.

sadasant · 2022-06-08T03:52:41Z

src/platform/common/utils/localize.ts

    );
    export const markdownHelpInstallingMissingDependencies = localize(
        'DataScience.markdownHelpInstallingMissingDependencies',
-        'See [https://aka.ms/pyaiinstall](https://aka.ms/pyaiinstall) for help on installing Jupyter and related dependencies.'
+        'See <https://aka.ms/pyaiinstall> for help on installing Jupyter and related dependencies.'


This seems equivalent for me.

sadasant · 2022-06-08T03:53:01Z

src/platform/common/utils/localize.ts

@@ -319,7 +355,7 @@ export namespace DataScience {
    );
    export const jupyterInstall = localize('DataScience.jupyterInstall', 'Install');
    export const currentlySelectedJupyterInterpreterForPlaceholder = localize(
-        'Datascience.currentlySelectedJupyterInterpreterForPlaceholder',
+        'DataScience.currentlySelectedJupyterInterpreterForPlaceholder',


Took the liberty to do some small improvements. Feedback appreciated!

sadasant · 2022-06-08T03:53:44Z

src/platform/common/utils/localize.ts

-    export const svgFilter = localize('DataScience.svgFilter', 'SVG');
+    export const pdfFilter = () => 'PDF';
+    export const pngFilter = () => 'PNG';
+    export const svgFilter = () => 'SVG';


I don't think these should be localized. Am I wrong?

sadasant · 2022-06-08T03:54:47Z

src/platform/common/utils/localize.ts

+        if (typeof key !== 'string') {
+            key = key.key;
+        }
+        return getString(key as string, defaultValue);


This does add a small performance impact, however it should be just constant, since the vscode-nls localize function is constant, as far as I understand.

sadasant · 2022-06-08T04:09:00Z

src/platform/common/utils/localize.ts

+        { key: 'DataScience.importDialogTitle', comment: ['{Locked="Notebook"}'] },
+        'Import Jupyter Notebook'
+    );
+    export const importDialogFilter = () => 'Jupyter Notebooks';


I'm assuming this would never change.

If Jupyter cannot change here, then it should be locked everywhere else.

I don't think Jupyter needs to be locked since it's an alteration, not a real word. It's also a subject name. I would assume it will be obvious that it shouldn't be translated.

codecov-commenter · 2022-06-08T04:38:22Z

Codecov Report

Merging #10361 (e05f2f5) into main (00c6329) will decrease coverage by 0%.
The diff coverage is 33%.

@@           Coverage Diff           @@
##            main   #10361    +/-   ##
=======================================
- Coverage     69%      69%    -1%     
=======================================
  Files        479      479            
  Lines      29453    29727   +274     
  Branches    5037     5040     +3     
=======================================
+ Hits       20544    20652   +108     
- Misses      7008     7175   +167     
+ Partials    1901     1900     -1

Impacted Files	Coverage Δ
...s-message-coordination/commonMessageCoordinator.ts	`68% <0%> (+7%)`	⬆️
src/kernels/jupyter/launcher/notebookProvider.ts	`75% <0%> (ø)`
.../raw/finder/localKnownPathKernelSpecFinder.node.ts	`89% <ø> (ø)`
src/platform/common/utils/localize.ts	`76% <ø> (-22%)`	⬇️
src/platform/errors/errorUtils.ts	`75% <ø> (ø)`
src/platform/messageTypes.ts	`100% <ø> (ø)`
...ide/dataviewer/dataViewerDependencyService.node.ts	`86% <100%> (ø)`
src/kernels/common/delayedFutureExecute.ts	`53% <0%> (-8%)`	⬇️
src/notebooks/outputs/tracebackFormatter.ts	`93% <0%> (-7%)`	⬇️
... and 6 more

rchiodo · 2022-06-08T16:49:18Z

package.nls.json

@@ -62,11 +62,11 @@
    "jupyter.command.jupyter.runLinting.title": "Run Linting",
    "jupyter.command.jupyter.runFileInteractive.title": "Run Current File in Interactive Window",
    "jupyter.command.jupyter.debugFileInteractive.title": "Debug Current File in Interactive Window",
-    "jupyter.command.jupyter.runallcells.title": "Run All Cells",
+    "jupyter.command.jupyter.runAllCells.title": "Run All Cells",


Is camel case required? Wondering why these changed.

I'll unchanged them, it just seemed like a good improvement to me. But it's not necessary.

I found plenty of others that you hadn't changed e.g. the line below the above one wasn't updated. If chaning I'd change it everywhere, or leave as is - or change in a seprate PR with a linter to ensure we are consistent (which I know we aren't)

I'll make an issue to make these consistent!

Issue: #10381

* Localization-tolerant gulp clean * a bit more general * much cleaner

sadasant · 2022-06-10T21:31:05Z

.vscodeignore

@@ -111,3 +112,4 @@ typings/**
 types/**
 test-results*
 *.zip
+!**/*nls.*.json


One of the challenges I had was that the *nls.*.json files were missing in the final .VSIX. This change, and another change in the gulpfile.js were the solution.

sadasant · 2022-06-10T21:31:26Z

TELEMETRY.md

@@ -1251,7 +1251,7 @@ No properties for event
 [src/notebooks/controllers/notebookControllerManager.ts](https://github.com/microsoft/vscode-jupyter/tree/main/src/notebooks/controllers/notebookControllerManager.ts)
 ```typescript
            }
-            // We know that this fails when we have xeus kernels installed (untill that's resolved thats one instance when we can have duplicates).
+            // We know that this fails when we have xeus kernels installed (until that's resolved thats one instance when we can have duplicates).


Took the liberty to fix some typos.

sadasant · 2022-06-10T21:33:00Z

build/azure-pipeline.pre-release.yml

@@ -22,6 +22,8 @@ resources:
 extends:
  template: azure-pipelines/extension/pre-release.yml@templates
  parameters:
+    locTsConfigs: $(Build.SourcesDirectory)/tsconfig.json
+    locBundleDestination: $(Build.SourcesDirectory)/out


As a reminder for future reviews of this PR, this should match the output path: in the webpack config: https://github.com/microsoft/vscode-jupyter/blob/main/build/webpack/webpack.extension.node.config.js#L142

sadasant · 2022-06-10T21:33:13Z

build/azure-pipeline.stable.yml

@@ -18,6 +18,8 @@ extends:
  template: azure-pipelines/extension/stable.yml@templates
  parameters:
    publishExtension: true
+    locTsConfigs: $(Build.SourcesDirectory)/tsconfig.json
+    locBundleDestination: $(Build.SourcesDirectory)/out


As a reminder for future reviews of this PR, this should match the output path: in the webpack config: https://github.com/microsoft/vscode-jupyter/blob/main/build/webpack/webpack.extension.node.config.js#L142

sadasant · 2022-06-10T21:33:40Z

build/webpack/webpack.extension.node.config.js

+                        options: {
+                            base: constants.ExtensionRootDir
+                        }
+                    },


We'll need to add this in the web webpack once there's web support.

sadasant · 2022-06-10T21:34:09Z

gulpfile.js

@@ -143,7 +143,7 @@ gulp.task('output:clean', () => del(['coverage']));

 gulp.task('clean:cleanExceptTests', () => del(['clean:vsix', 'out', '!out/test']));
 gulp.task('clean:vsix', () => del(['*.vsix']));
-gulp.task('clean:out', () => del(['out/**', '!out', '!out/client_renderer/**']));
+gulp.task('clean:out', () => del(['out/**', '!out', '!out/client_renderer/**', '!**/*nls.*.json']));


One of the challenges I had was that the *nls.*.json files were missing in the final .VSIX. This change, and another change in the .vscodeignore were the solution.

…the browser

sadasant · 2022-06-10T22:08:45Z

src/platform/common/utils/localize.ts

-        'Common.clickHereForMoreInfoWithHtml',
-        "Click <a href='{0}'>here</a> for more info."
-    );
+    export const bannerLabelYes = () => localize('Common.bannerLabelYes', 'Yes');


Changed these to be functions to allow keeping the old behavior in the browser. At first, I had this change within a replacement of the localize function itself, but that broke the matching mechanism used by vscode-nls-dev. I've got to allow the Node logic to assume localize is unchanged, and thus to keep the signature in the other files, these properties now are lambdas.

I will fix this once vscode-nls supports the web.

sadasant · 2022-06-10T22:10:03Z

src/platform/common/utils/localize.ts

@@ -1262,6 +1436,8 @@ function getString(key: string, defValue?: string) {
        collection = loadedCollection;
    }
    let result = collection[key];
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    result = (result as any)?.message || result;


Since package.nls.json now can return objects, we should retrieve the message from those objects in such cases.

sadasant · 2022-06-10T22:14:11Z

@rchiodo , @DonJayamanne This PR is ready 😆 fully tested. One last review appreciated 🙏

sadasant · 2022-06-11T14:56:57Z

I'm getting some weird errors in the build, like:

FAILED TEST NAME: DataScience - VSCode Notebook - (Conda Execution) (slow) Acitvate conda environment using conda run and activation commands.
FAILED TEST NAME: DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow) Interrupt and running cells again should only run the necessary cells.

Investigating...

sadasant · 2022-06-11T21:01:09Z

src/test/datascience/interpreters/environmentActivationService.vscode.test.ts

@@ -110,7 +110,7 @@ suite('DataScience - VSCode Notebook - (Conda Execution) (slow)', function () {
        );
        verifyVariables(envVarsOurselves!, '(ourselves)');
    });
-    test('Acitvate conda environment using conda run and activation commands', async () => {
+    test('Activate conda environment using conda run and activation commands', async () => {


Could this typo fix be affecting a recording or a CI setting of some sort?

I wouldn't think so. Have you checked some recent runs from main / other PRs to see if that test is failing in them?

Yeah this is a common failure. I'm trying to fix this here:
#10429

sadasant added 10 commits June 6, 2022 16:01

wip

df2d5d6

all the API changes in localize.ts, I believe

4b3c115

some of the file changes

a1abab3

wip

ee6c2ad

reinstating the .nls.*.json files

dfddccb

kernel entries and other stuff

f91ca25

typos

373078f

Notebook Renderers

69ac25e

wip

eeecdab

webpack fixes

ae5f0b0

sadasant self-assigned this Jun 8, 2022

sadasant requested a review from a team as a code owner June 8, 2022 03:46

sadasant commented Jun 8, 2022

View reviewed changes

src/platform/common/utils/localize.ts Outdated Show resolved Hide resolved

sadasant commented Jun 8, 2022

View reviewed changes

Merge remote-tracking branch 'VSCode/main' into debt/9676

3956675

sadasant commented Jun 8, 2022

View reviewed changes

rchiodo reviewed Jun 8, 2022

View reviewed changes

sadasant and others added 7 commits June 10, 2022 12:37

Exception in .vscodeignore for nls.bundle.*.json (microsoft#10415)

4a3d240

test without clean (microsoft#10421)

3f9d033

Localization-tolerant gulp clean (microsoft#10423)

d09787b

* Localization-tolerant gulp clean * a bit more general * much cleaner

Better nls.*.json regexes (microsoft#10425)

a2d7920

Merge branch 'debt/9676-2' into debt/9676

8daa92f

Merge remote-tracking branch 'VSCode/main' into debt/9676

2ce3f33

better comment

2c0df33

sadasant commented Jun 10, 2022

View reviewed changes

package.nls.json now has object values -- this fixes localization in …

27c5f92

…the browser

sadasant commented Jun 10, 2022

View reviewed changes

rchiodo approved these changes Jun 10, 2022

View reviewed changes

IanMatthewHuff approved these changes Jun 10, 2022

View reviewed changes

sadasant added 5 commits June 10, 2022 19:08

test fix

a6e3152

small changes for consistency

40114ad

consistent quotes

a31944f

undoing some unnecessary change

a726bc5

This could be the fix

97bf0bd

sadasant added 2 commits June 11, 2022 13:17

some polishes that seemed reasonable to me

bf36972

Merge remote-tracking branch 'VSCode/main' into debt/9676

e05f2f5

sadasant commented Jun 11, 2022

View reviewed changes

sadasant merged commit 039cf29 into microsoft:main Jun 13, 2022

sadasant deleted the debt/9676 branch June 13, 2022 16:27

Localize using vscode-nls #10361

Localize using vscode-nls #10361

Uh oh!

Conversation

sadasant commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant commented Jun 10, 2022

Uh oh!

sadasant commented Jun 11, 2022

Uh oh!

sadasant commented Jun 8, 2022 •

edited

Loading

sadasant Jun 8, 2022 •

edited

Loading

sadasant Jun 8, 2022 •

edited

Loading

sadasant Jun 8, 2022 •

edited

Loading

codecov-commenter commented Jun 8, 2022 •

edited

Loading

IanMatthewHuff Jun 13, 2022 •

edited

Loading