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

Added ability for JSON/XLIFF conversion and Push/Pull from Transifex #23281

Merged
merged 21 commits into from
Apr 6, 2017
Merged

Added ability for JSON/XLIFF conversion and Push/Pull from Transifex #23281

merged 21 commits into from
Apr 6, 2017

Conversation

michelkaporin
Copy link
Contributor

Wrote a JSON->XLIFF parser tackling #22092, and implemented XLIFFs push/pull of translations using Transifex API for #22070

@michelkaporin michelkaporin added this to the March 2017 milestone Mar 27, 2017
@michelkaporin michelkaporin requested a review from dbaeumer March 27, 2017 13:52
@mention-bot
Copy link

@michelkaporin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @egamma and @alexandrudima to be potential reviewers.

@msftclas
Copy link

@michelkaporin,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@@ -85,6 +299,26 @@ const iso639_3_to_2: Map<string> = {
'trk': 'tr'
};

const iso639_2_to_3: Map<string> = {
Copy link
Member

Choose a reason for hiding this comment

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

We might want to consider using 2 letter id in the future. The reason why we have this mapping is that MS internally uses 3. If all is public we could switch over to 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created #23919 for it.

promise = createResource(project, slug, file, apiHostname, credentials);
}
updateCreatePromises.push(promise);
}).catch((reason) => {
Copy link
Member

Choose a reason for hiding this comment

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

I am not a gulp expert either but I think this simply logs something to the console but the gulp stream says in tact and the build will look like it passed. We have to ensure that in the cause of exceptions talking to Transifex this can be observed as a failing build from the outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the pipeline to fail whenever we have any parsing or Transifex push/pull errors.

'ita': 'CP1252'
};

export function createIslFile(base: string, originalFilePath: string, messages: Map<string>, language: string): File {
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider that for the ISL we only need to push ch* and kor. For all other languages we have translations. Or is this not possible since languages are defined by a Transifex project. If so we might want to consider not pushing the Default.isl to Transifex

Copy link
Member

Choose a reason for hiding this comment

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

We should consider a vscode-setup project to control better to which languages we need the Default.isl translated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vscode-setup project was created on Transifex, the code amended correspondingly, so that we now pull only ch* and kor languages.

@dbaeumer
Copy link
Member

dbaeumer commented Apr 6, 2017

It's ok for me to merge.

@michelkaporin michelkaporin merged commit 96bd2ed into microsoft:master Apr 6, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants