Refactor format of l10n strings to standard format

[rudyflores]

Proposed changes

Refactored format of l10n strings to use the standard vscode.l10n.t(message, args) format instead of the vscode.l10n.t({ message, args, comments }) format, which introduced issues on certain cases.

Release Notes

Milestone: v3.0.0

Changelog: Changed format of l10n strings to standard format

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Enhancement (non-breaking change which adds or improves functionality)
• Breaking change (a change that would cause existing functionality to not work as expected)
• Documentation (Markdown, README updates)
• Other (please specify above in "Proposed changes" section)

Checklist

General

• I have read the CONTRIBUTOR GUIDANCE wiki
• All PR dependencies have been merged and published (if applicable)
• A GIF or screenshot is included in the PR for visual changes
• The pre-publish command has been executed:
  • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
  • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

• There is coverage for the code that I have added
• I have added new test cases and they are passing
• I have manually tested the changes

Deployment

• I have added developer documentation (if applicable)
• Documentation should be added to Zowe Docs
  • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
  • Otherwise, please check with the rest of the squad about any needed documentation before merging.
• These changes may need ported to the appropriate branches (list here):

Further comments

[github-actions]

📅 Suggested merge-by date: 10/7/2024

[JillieBeanSim]

Thanks @rudyflores for this localization fix. Should we also update the items in JobTableView.ts as well or does it behave differently in table views?

[rudyflores]

Thanks @rudyflores for this localization fix. Should we also update the items in JobTableView.ts as well or does it behave differently in table views?

I believe this might be a potential issue too, thanks for finding that! Not sure why that didn't appear when searching :o

[codecov]

Codecov Report

Attention: Patch coverage is 89.56522% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (3c524b8) to head (3516469).
Report is 86 commits behind head on main.

Files with missing lines	Patch %	Lines
...ckages/zowe-explorer/src/configuration/Profiles.ts	70.00%	3 Missing ⚠️
...es/zowe-explorer/src/commands/MvsCommandHandler.ts	33.33%	2 Missing ⚠️
...es/zowe-explorer/src/commands/TsoCommandHandler.ts	33.33%	2 Missing ⚠️
...s/zowe-explorer/src/commands/UnixCommandHandler.ts	66.66%	2 Missing ⚠️
packages/zowe-explorer/src/trees/job/JobActions.ts	90.90%	1 Missing ⚠️
packages/zowe-explorer/src/trees/uss/USSActions.ts	50.00%	1 Missing ⚠️
...ackages/zowe-explorer/src/trees/uss/ZoweUSSNode.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3137   +/-   ##
=======================================
  Coverage   92.55%   92.56%           
=======================================
  Files         113      113           
  Lines       11649    11640    -9     
  Branches     2577     2541   -36     
=======================================
- Hits        10782    10774    -8     
+ Misses        865      864    -1     
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zFernand0]

Does this mean that we are removing the context in POEditor for those contributing translations?

I might be confusing things here though. Here is what VSCode documents:

[traeok]

Does the previous function call not work for us due to issues with translations or our own logic, or is this something that could be resolved upstream in microsoft/vscode-l10n?

I tried debugging this issue (changing languages with the translations in place), but it seems like the error occurs from within the vscode-l10n package itself.

[JillieBeanSim]

Does the previous function call not work for us due to issues with translations or our own logic, or is this something that could be resolved upstream in microsoft/vscode-l10n?

I tried debugging this issue (changing languages with the translations in place), but it seems like the error occurs from within the vscode-l10n package itself.

If it is found to be upstream, should we have the workaround in place until fixed upstream?

[traeok]

If it is found to be upstream, should we have the workaround in place until fixed upstream?

Definitely in favor of implementing a workaround until it can be resolved upstream - if it hasn't been filed as a bug in the l10n repo, we could make an issue with steps to reproduce.

That said, if we're removing context from translators within POEditor, then it might be easier to wait for the fix so that we don't have to add the comments back once its resolved. If the comments are unused, then feel free to disregard - we can always keep the changes in place.

[t1m0thyj]

Does this mean that we are removing the context in POEditor for those contributing translations?

I might be confusing things here though. Here is what VSCode documents:

@zFernand0 This should not affect the context value shown in POEditor. The context is a translation ID defined only for strings coming from package.nls.json. The comments are unsupported/unused by POEditor as far as I know 😋

@rudyflores Thanks for the refactor! Are you able to provide more details about "issues on certain cases" to help with testing this PR?

[rudyflores]

If it is found to be upstream, should we have the workaround in place until fixed upstream?

Definitely in favor of implementing a workaround until it can be resolved upstream - if it hasn't been filed as a bug in the l10n repo, we could make an issue with steps to reproduce.

That said, if we're removing context from translators within POEditor, then it might be easier to wait for the fix so that we don't have to add the comments back once its resolved. If the comments are unused, then feel free to disregard - we can always keep the changes in place.

I think we should definitely have this workaround in place, otherwise we can't provide localization for any user's outside of english when we did in the past, which means we are breaking in terms of the support for that :/

[rudyflores]

Does this mean that we are removing the context in POEditor for those contributing translations?
I might be confusing things here though. Here is what VSCode documents:

@zFernand0 This should not affect the context value shown in POEditor. The context is a translation ID defined only for strings coming from package.nls.json. The comments are unsupported/unused by POEditor as far as I know 😋

@rudyflores Thanks for the refactor! Are you able to provide more details about "issues on certain cases" to help with testing this PR?

Yes! @t1m0thyj so it fails if you change an l10n string to use the object format, so for example if you do that for any l10n string in our activation code and switch the language, Zowe Explorer will fail to activate

[sonarqubecloud]

Quality Gate failed

Failed conditions
7.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

[phaumer]

Can confirm that it now work. I posted updated translations in Slack.

[JillieBeanSim]

LGTM! thanks @rudyflores for the fix

[t1m0thyj]

I don't know if we want to remove all the l10n comments from our code. The npm docs for @vscode/l10n-dev suggest that using comments is good practice, but they need to be stripped out in the translated bundle file:

For ZE, the downloadPoeditorL10n.js script already handles this - when downloading translations from POEditor it creates l10n bundle files in a format that has the comments removed. For testing purposes we could add a small script to the repo that strips out comments for any l10n bundle file:

const fs = require("fs");
const bundleJsonFile = process.argv[2];
const l10nBundle = JSON.parse(fs.readFileSync(bundleJsonFile, "utf-8"));
for (const [k, v] of Object.entries(l10nBundle)) {
    if (typeof v === "object") {
        l10nBundle[k] = l10nBundle[k].message;
    }
}
fs.writeFileSync(bundleJsonFile, JSON.stringify(l10nBundle, null, 2) + "\n");

If this would be a satisfactory solution, I'd prefer to keep the l10n comments in our code as they may add value for human translators 😋

[rudyflores]

I don't know if we want to remove all the l10n comments from our code. The npm docs for @vscode/l10n-dev suggest that using comments is good practice, but they need to be stripped out in the translated bundle file:

For ZE, the downloadPoeditorL10n.js script already handles this - when downloading translations from POEditor it creates l10n bundle files in a format that has the comments removed. For testing purposes we could add a small script to the repo that strips out comments for any l10n bundle file:

const fs = require("fs");
const bundleJsonFile = process.argv[2];
const l10nBundle = JSON.parse(fs.readFileSync(bundleJsonFile, "utf-8"));
for (const [k, v] of Object.entries(l10nBundle)) {
    if (typeof v === "object") {
        l10nBundle[k] = l10nBundle[k].message;
    }
}
fs.writeFileSync(bundleJsonFile, JSON.stringify(l10nBundle, null, 2) + "\n");

If this would be a satisfactory solution, I'd prefer to keep the l10n comments in our code as they may add value for human translators 😋

Thoughts on this? @phaumer @JillieBeanSim

[phaumer]

I cannot change the output format of our automated translation. If there is content that breaks it then we indeed need to remove from the translation input files.

[t1m0thyj]

I cannot change the output format of our automated translation. If there is content that breaks it then we indeed need to remove from the translation input files.

We could enhance the ZE npm script generateLocalization to strip out comments using the process I described above so that the translation input file consumed by the pipeline would already be in the right format.

Edit: Here's an example of how that would look: 65c1f61 In that commit I've kept the l10n comments in code but stripped them from the json file.

[t1m0thyj]

Should we close this in favor of #3149?

[rudyflores]

Should we close this in favor of #3149?

@phaumer what do you think of this approach instead?

[zFernand0]

Should we close this in favor of #3149?

@phaumer what do you think of this approach instead?

Hey all,
Now that #3149 is merged, is this PR ok to close for now?

[zFernand0]

Waiting on a decision (on whether to close this PR) before reviewing 😋

[JTonda]

Closing in favor of #3149