-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add localization cli command #10187
Add localization cli command #10187
Conversation
5b99efd
to
9decf1e
Compare
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.
Very cool! Works like a charm.
9decf1e
to
b81080a
Compare
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.
@brianking @marcdumais-work just as a precaution, I'd like your input about using deepl (which is a free or paid service that requires an api key and credit card) in order to perform translations. Is there anything from the eclipse foundation point of view which discourages doing such things?
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.
@msujew can you clarify why we want to translate json
files exclusively?
dev-packages/cli/package.json
Outdated
@@ -31,6 +31,7 @@ | |||
"dependencies": { | |||
"@theia/application-manager": "1.18.0", | |||
"@theia/application-package": "1.18.0", | |||
"@theia/localization-manager": "1.18.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.
@msujew the use of deepl
in the extension makes me think once again how we should write @theia/cli
in a way that we can contribute functionality or scripts to it, without the need to depend on extensions explicitly. Not only will it help with coupling, but applications would have more control over what functionality they want to include in their products, especially for a case like deepl
which they might no want to.
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.
Right, that makes sense. I'll leave this PR out of the merging round I am planning for today for the localization PRs, as it's not absolutely necessary for localization support of Theia.
@vince-fugnitto There no real reason to support json files exclusively. However, as the output format from the Nonetheless, I am open to the idea to add additional input and output formats. |
@msujew sorry, I had understood that it only translated |
We don't. The |
@msujew unfortunately even something as simple as |
@vince-fugnitto Actually, the only info that's sent to deepl is the default value. The mapping to the id only happens in the Anyway, the main idea behind the deepl integration was to easily translate Theia itself (#9708). While it can also be used by downstream users, they are free to use other services. Furthermore - at least for paying customers - deepl promises high confidentiality for the data sent to them. |
b81080a
to
50daeb0
Compare
@vince-fugnitto The DeepL related CQ has been approved. I rebased my changes for further reviewing. |
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.
@msujew can you describe a bit how the deepl
integration is supposed to work for theia translations (perhaps in the readme of the extension). It's not instantly clear to me what it does or how it works. Some insights on how translations of theia should work, and if we should maintain some type of theia nls.metada.json
in the framework for our own translations.
50daeb0
to
8c35d41
Compare
@vince-fugnitto I expanded the readme and added an example. I would refrain from adding more documentation in there, instead choosing to dedicate a larger section to it in the internationalization documentation. Regarding, a |
@msujew I'd be fine with whatever you propose :) the important thing is we have some form of documentation to help developers, and have a clear way forward. |
@vince-fugnitto : Are you fine with merging this? |
Uses the machine translation API of DeepL to automatically translate any missing values
Uses the machine translation API of DeepL to automatically translate any missing values
8c35d41
to
0a4da65
Compare
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.
LGTM! @msujew I don't have any further comments regarding the code 👍
@vince-fugnitto Thanks, I'll merge it after the release 👍 |
@msujew : Release is done, "ping" :-) |
@msujew Can this be merged? |
What it does
Closes #9708
Uses the machine translation API of DeepL to automatically translate json files.
How to test
Testing this PR requires a DeepL API key. While 500k characters/month are free for developers, even a free account requires a credit card (claiming to reduce the amount of multi-accounts that abuse the free dev-tier). If you want to review this PR but don't have access to an account already, or are hesitant to use your own credit card there, you can send me a mail (linked in my github profile) and I'll send you an API key.
help
of the localize commandReview checklist
Reminder for reviewers