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

Simplify localization for vscode related strings #10319

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

msujew
Copy link
Member

@msujew msujew commented Oct 24, 2021

What it does

Related to the discussions that occurred in #10106 and #10299,

Heavily simplifies the localization process for strings that are contained inside of vscode. Additionally removes the coupling between vscode internals and Theia's translations calls.

How it works

Utilizes the nls.metadata.json that is generated by the vscode build process (in this case the 1.53.2 version of vscode). It can be produced by running yarn && yarn gulp vscode-linux-x64 inside of the vscode repository. This files contains every default value and its key of the vscode nls.localize calls that occur inside of the vscode codebase. With a little bit of transformation work, we can produce a map that maps every default value to the associated key.

This has the drawback of commiting/polluting the Theia repository with this generated file. I'm open to suggestions on how to improve this.

How to test

  1. Open Theia in english and observe that everything continues running as usual
  2. Install a language pack of your choice and run the Configure Display Language command.
  3. Assert that the application is (mostly) translated into the target language, even without explicitly referencing vscode keys.

Review checklist

Reminder for reviewers

@msujew msujew added localization issues related to localization/internalization/nls quality issues related to code and application quality labels Oct 24, 2021
@msujew
Copy link
Member Author

msujew commented Oct 24, 2021

@vince-fugnitto How is the CQ/Licensing guideline in regards to generated files? As the nls.metadata.json file is generated using data that's inside of the vscode codebase.

@msujew msujew force-pushed the msujew/simplify-localization branch from ecbb88b to c19e621 Compare October 24, 2021 15:24
@vince-fugnitto
Copy link
Member

@vince-fugnitto How is the CQ/Licensing guideline in regards to generated files? As the nls.metadata.json file is generated using data that's inside of the vscode codebase.

I'm not 100% sure what the procedure should be in a case like this, but if we want to be safe we can always register a CQ. Perhaps @marcdumais-work can shed some light.

@msujew msujew mentioned this pull request Oct 25, 2021
8 tasks
@marcdumais-work
Copy link
Contributor

I agree that opening a CQ is the best way forward. At first approach, I would treat it like copied code. Have you opened a CQ before, @msujew?

@msujew
Copy link
Member Author

msujew commented Oct 25, 2021

@marcdumais-work I haven't yet, but I assume following the steps outlined in your link will get me there successfully?

@vince-fugnitto
Copy link
Member

@marcdumais-work I haven't yet, but I assume following the steps outlined in your link will get me there successfully?

@msujew yes, more specifically it would be this step: https://github.com/eclipse-theia/theia/wiki/Registering-CQs#codecontent-is-copied-from-other-projectsrepos.

@marcdumais-work
Copy link
Contributor

Maybe you can ask if a CQ will be needed every time we uplift this file, or how/what we can check ourselves.

@msujew
Copy link
Member Author

msujew commented Oct 25, 2021

@vince-fugnitto I filed a CQ and updated the PR 👍

@msujew msujew force-pushed the msujew/simplify-localization branch from c19e621 to 2838329 Compare November 2, 2021 13:02
@msujew
Copy link
Member Author

msujew commented Nov 2, 2021

@vince-fugnitto Although the CQ isn't finished yet, the file has been approved to be merged into the codebase. So this is ready for a review 👍

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@msujew overall the changes look much better, and I suspect will be much easier to write for developers going forward 👍

As for nls.metadata.json, how often to we expect to uplift the file, is it something we should do regularly (following a release)?

I know that @marcdumais-work asked the question before (#10319 (comment)):

Maybe you can ask if a CQ will be needed every time we uplift this file, or how/what we can check ourselves.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

All of the functionality looks good to me - everything that (I think) should be localized is localized. A few comments / questions.

packages/core/src/common/nls.ts Show resolved Hide resolved
packages/core/src/common/nls.ts Show resolved Hide resolved
packages/core/src/common/nls.ts Outdated Show resolved Hide resolved
@msujew msujew force-pushed the msujew/simplify-localization branch 3 times, most recently from e8a1950 to 802af70 Compare November 2, 2021 15:27
@msujew
Copy link
Member Author

msujew commented Nov 2, 2021

As for nls.metadata.json, how often to we expect to uplift the file, is it something we should do regularly (following a release)?

@vince-fugnitto The file is currently in sync with the assumed version of the installed language pack, which is currently 1.53.2. I used the 1.53.2 build of vscode to generate the file as well. Once we lift the vscode API version in Theia, this file should be lifted to the same version as well.

It shouldn't be an issue if users install newer versions of language packs, as vscode rarely removes/changes localization IDs (as in: I haven't observed it yet). However, we can't use a newer version of the nls.metadata.json in Theia, because we run the risk of using strings/default values that translate into keys which don't exist in the language packs that are currently installable in Theia.

@msujew
Copy link
Member Author

msujew commented Nov 4, 2021

@vince-fugnitto @colin-grant-work FYI, as a next step, I plan to add a eslint rule that inspects localize and localizeByDefault calls to identify potential missuses of each method using the nls.metadata.json file (i.e. localizing text inside of localize that belongs to localizeByDefault and vice-versa).

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

CI is now green, and I think that this way of localizing things will be much easier to handle, and much easier to expand with non-VSCode translations where necessary.

@msujew msujew force-pushed the msujew/simplify-localization branch from 802af70 to 5365670 Compare November 5, 2021 10:53
Copy link
Member

@vince-fugnitto vince-fugnitto 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 fine with the changes, there is now a much better dev experience 👍
Once merged there might be PRs that need to be re-updated so I'll keep an eye out for them, and we probably want to update the coding guidelines once again.

@msujew
Copy link
Member Author

msujew commented Nov 5, 2021

@vince-fugnitto Thanks, I'll try to keep an eye out as well 👍

@msujew msujew merged commit 39fe372 into master Nov 5, 2021
@msujew msujew deleted the msujew/simplify-localization branch November 5, 2021 13:11
@github-actions github-actions bot added this to the 1.20.0 milestone Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
localization issues related to localization/internalization/nls quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants