-
Notifications
You must be signed in to change notification settings - Fork 264
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
Send DeepL translation requests in multiple batches #474
Conversation
…eepl's target locale requirement for EN and PT which need their sub-version defined e.g. en-us
def to_deepl_target_locale(locale) | ||
loc, sub = locale.to_s.split('-') | ||
if SPECIFIC_TARGETS.include?(loc) | ||
fail ::I18n::Tasks::CommandError, I18n.t('i18n_tasks.deepl_translate.errors.specific_target_missing') unless sub |
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 API docs say that the the language without a region is supported for backwards compatibility. Perhaps we should issue a warning instead?
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.
yes that may be a bit nicer, i'll convert that to a warn_deprecated, as thats what i found in other regions of the gem.
Btw. i get an error using a deepl pro key for the spanish test translations:
are you running those test too maybe with the free api and it has a different output? Else i would fix those tests too. |
Yeah these tests can be a bit flaky. Feel free to update |
…ould be a bug on their side of we may need another tag-handling param
By the way, we're not currently handling placeholders correctly. This is tracked in #417 |
Thanks for merging!
Thanks for the hint, i'll keep an ey on that, so far i had not had any exceptions my target app. Hard to tell how deepl maybe fixed any issues. |
I did get an unspecified error when sending 2000+ translations to deepl. The author of the deepl gem has relased a new version where he added an specific error when hitting a limit.
This PR also handles deepl's target locale requirement for EN and PT which need their sub-version defined e.g. EN-US.
This can be a potential 'breaking' change for users still using EN as plain target, as i added an error for this case. See https://www.deepl.com/de/docs-api/translate-text/translate-text/ with their target_lang description