-
Notifications
You must be signed in to change notification settings - Fork 676
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
Onboard Localization pipeline #5990
Conversation
@@ -1322,7 +1329,7 @@ | |||
"properties": { | |||
"dotnet.defaultSolution": { | |||
"type": "string", | |||
"description": "The path of the default solution to be opened in the workspace, or set to 'disable' to skip it.", | |||
"description": "%configuration.dotnet.defaultSolution.description%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample used in this PR
@@ -494,7 +494,7 @@ export class RoslynLanguageServer { | |||
return; | |||
} | |||
|
|||
const title = 'Restart Language Server'; | |||
const title = vscode.l10n.t('Restart Language Server'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other sample used in this PR
"LanguageSet": "VS_Main_Languages", | ||
"LocItems": [ | ||
{ | ||
"SourceFile": "./loc/vscode-csharp.xlf", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vscode-csharp.xlf
is a generated file from l10nDevGenerateXlf
task in package.json.
It is passed to the localization build.
./loc folder is used to stage all the generated files.
tasks/localizationTasks.ts
Outdated
const lsRemote = await git(['ls-remote', remoteRepoAlias, 'refs/head/' + newBranchName]); | ||
if (lsRemote.trim() !== '') { | ||
// If the localization branch of this commit already exists, don't try to create another one. | ||
console.log(`${newBranchName} already exists in ${parsedArgs.targetRemoteRepo}. Skip pushing.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this expected to happen? Should it throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this might happen, in case the pipeline run is canceled when branch is pushed but no PR is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets log this as an actual pipeline error
https://learn.microsoft.com/en-us/azure/devops/pipelines/scripts/logging-commands?view=azure-devops&tabs=powershell#logissue-log-an-error-or-warning
.filter((fileName) => fileName.length !== 0); | ||
} | ||
|
||
async function git(args: string[], printCommand = true): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we always print the command, so maybe removable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, when we call git add remote url
, PAT is encoded like this
https://${parsedArgs.userName}:${pat}@github.com/${parsedArgs.owner}/${parsedArgs.targetRemoteRepo}.git
I don't want to reveal the PAT in this case
Because Phil merged this #5962 so there are more strings need localization : ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good now - I think still just pending if we can avoid the diff reset in https://github.com/dotnet/vscode-csharp/pull/5990/files#r1282385281
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 minor comment
tasks/localizationTasks.ts
Outdated
const lsRemote = await git(['ls-remote', remoteRepoAlias, 'refs/head/' + newBranchName]); | ||
if (lsRemote.trim() !== '') { | ||
// If the localization branch of this commit already exists, don't try to create another one. | ||
console.log(`${newBranchName} already exists in ${parsedArgs.targetRemoteRepo}. Skip pushing.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets log this as an actual pipeline error
https://learn.microsoft.com/en-us/azure/devops/pipelines/scripts/logging-commands?view=azure-devops&tabs=powershell#logissue-log-an-error-or-warning
@@ -0,0 +1,85 @@ | |||
{ | |||
"Cannot load Razor language server because the directory was not found: '{0}'": "Cannot load Razor language server because the directory was not found: '{0}'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cosifne The keys
of this file can be used to help organize who owns the strings / help what the usage should be. I would reccomend changing it to be something similar so its easier to use rather than having to change they key and value in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key is auto-generated by the l10n tool, let me see if this can be optimized or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cosifne Sorry I'm incorrect here. I think this is correct for the l10n.json
file. I got this confused with the nls.json
file.
Sample run #5987
https://dev.azure.com/dnceng/internal/_build/results?buildId=2234885&view=results
This PR contains two localization file changes. One in code and one in package.json as the test sample.
In code, it uses vs-l10n to do localization.
The pipeline works like this:
l10n
folder. (l10n-dev)main
on GitHub.I feel it is enough to always translate our main branch here. If there is any problem we can address it in the future.
After this, two more things are needed.