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

add DeepL Pro artificial intelligence translation service #294

Merged
merged 7 commits into from
Aug 1, 2018

Conversation

neumayr
Copy link
Contributor

@neumayr neumayr commented Jul 30, 2018

Hey @glebm ๐Ÿ‘‹

  • add DeepL Pro artificial intelligence translation service โœจ
  • use deepl-rb gem for query the API ๐Ÿ’Ž
    • Kudos to @wikiti for helping with the latest v2.1.0 release that adds ignore_tags ๐Ÿ’ช
Update August 2018

DeepL is working to add the translation for ๐Ÿ‡ต๐Ÿ‡น ๐Ÿ‡ท๐Ÿ‡บ ๐Ÿ‡ฏ๐Ÿ‡ต ๐Ÿ‡จ๐Ÿ‡ณ but didn't specify a fixed release date yet

@neumayr neumayr force-pushed the deepl-translation branch from 4ae1429 to b31b573 Compare July 30, 2018 10:31
# @return [Array<[String, Object]>] translated list
def fetch_translations(list, opts)
options = {
tag_handling: 'xml',
Copy link
Owner

Choose a reason for hiding this comment

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

Should it have tag_handling: 'xml' for non-HTML keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx ๐Ÿ‘

README.md Outdated
```yaml
# config/i18n-tasks.yml
translation:
api_key: <Deep Pro API key>
Copy link
Owner

Choose a reason for hiding this comment

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

Let's have separate settings for different API keys. google_translate_api_key and deepl_api_key. (api_key can then work as google_translate_api_key with a deprecation warning`)

@@ -46,6 +47,20 @@ def translate_missing(opt = {})
print_forest translated, opt
end

# DeepL Pro Translate
cmd :translate_missing_deepl,
Copy link
Owner

@glebm glebm Jul 30, 2018

Choose a reason for hiding this comment

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

Let's add a --backend flag to translate_missing instead of having a separate command that is otherwise identical.

The argument definition can look something like this:

TRANSLATION_BACKENDS = %w[google deepl].freeze
DEFAULT_TRANSLATION_BACKEND = TRANSLATION_BACKENDS[0]
arg :translation_backend,
    '-b',
    '--backend BACKEND',
    TRANSLATION_BACKENDS,
    t('i18n_tasks.cmd.args.desc.translation_backend',
      valid_text: TRANSLATION_BACKENDS * ', ', default_text: DEFAULT_TRANSLATION_BACKEND),
    parser: OptionParsers::Enum::Parser.new(
      values, ->(value, valid) { I18n.t('i18n_tasks.cmd.errors.invalid_format',
                                        invalid: value, valid: TRANSLATION_BACKENDS * ', ')})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you check if Command::Options::Locales could be good as well?


# @param [Array<[String, Object]>] list of key-value pairs
# @return [Array<[String, Object]>] translated list
def translate_list(list, opts) # rubocop:disable Metrics/AbcSize
Copy link
Owner

@glebm glebm Jul 30, 2018

Choose a reason for hiding this comment

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

Consider prefixing all methods here and in google_translation.rb with google_ / deepl_ and extracting common methods into a base_translation.rb module.

Alternatively, extract all the logic from fetch_translations and below to a class to avoid namespace conflicts. A base class with two descendants is also OK.

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 prefixed the methods with google_ / deepl_. The only method is *_to_values(list) that would identify for a base_translation.rb Do you think it's worth sharing?

Copy link
Owner

Choose a reason for hiding this comment

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

Not worth it unless we re-structure it to share more methods. I can look into it after merging, thanks.

@glebm
Copy link
Owner

glebm commented Jul 30, 2018

Thanks @neumayr, I've left some comments.

neumayr added 5 commits July 31, 2018 16:31
* add [DeepL Pro](https://www.deepl.com/pro) artificial intelligence translation service
* use [`deepl-rb`](https://github.com/wikiti/deepl-rb) gem for query the API
* currently the cmd is `i18n-tasks translate-missing-deepl` because I wasnโ€™t sure how you would structure the commands with multiple vendors
* separate translation settings into different API keys
  * `google_translate_api_key`
  * `deepl_api_key`
* add `โ€”backend` flag to `translate_missing` command
* prefix google methods with `google_`
@neumayr neumayr force-pushed the deepl-translation branch from 57aefc7 to 0bdbaa1 Compare July 31, 2018 14:32
@neumayr
Copy link
Contributor Author

neumayr commented Jul 31, 2018

@glebm Ready for another review round

*  run `i18n-tasks normalize`
# @return [Array<[String, Object]>] translated list
def fetch_translations(list, opts)
options = {
ignore_tags: %w[i18n]
Copy link
Owner

Choose a reason for hiding this comment

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

Does ignore_tags work even without tag_handling: 'xml'?
If not, we may need to use the same trick as in the Google Translate implementation

Copy link
Contributor Author

@neumayr neumayr Aug 1, 2018

Choose a reason for hiding this comment

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

Yes, ignore_tags works even without other params. And should be fixed,
because %{user} interpolation can be with _html / .html keys and inside simple text tags too.

๐Ÿ“ https://www.deepl.com/api.html

@glebm glebm merged commit 46fb95c into glebm:master Aug 1, 2018
@glebm glebm deleted the deepl-translation branch August 1, 2018 18:09
@glebm
Copy link
Owner

glebm commented Aug 1, 2018

Thanks, merged! Can you email me the key so I may add it to Travis?

glebm added a commit that referenced this pull request Aug 1, 2018
Adds prefixes to stragglers.
Changelog.
glebm added a commit that referenced this pull request Aug 1, 2018
@glebm
Copy link
Owner

glebm commented Aug 1, 2018

I've moved the shared code to a base class in 0002e80

glebm added a commit that referenced this pull request Aug 1, 2018
glebm added a commit that referenced this pull request Aug 1, 2018
glebm added a commit that referenced this pull request Aug 1, 2018
glebm added a commit that referenced this pull request Aug 3, 2018
Follow-up to #294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants