-
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
deepl plural and singular response handling #347
Conversation
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'd have suggested the same.
What do you think about adding a regression test to deepl_translate_spec.rb
? Especially because the API of deepl-rb
specifically uses 1-in-1-out and many-in-many-out.
Sounds good. Any idea how to add it ?
…On Mon 9. Nov 2020 at 13:22, Josua Schmid ***@***.***> wrote:
***@***.**** approved this pull request.
I'd have suggested the same.
What do you think about adding a regression test to
deepl_translate_spec.rb?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#347 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAK7CTCGYEAH5OZO4G2IP73SO7NIPANCNFSM4MDZBIEQ>
.
|
@@ -17,7 +17,12 @@ def initialize(*) | |||
protected | |||
|
|||
def translate_values(list, from:, to:, **options) | |||
DeepL.translate(list, to_deepl_compatible_locale(from), to_deepl_compatible_locale(to), options).map(&:text) | |||
deepl = DeepL.translate(list, to_deepl_compatible_locale(from), to_deepl_compatible_locale(to), options) | |||
if deepl.respond_to? 'map' |
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.
Does it respond with a string when there is only one element in the list?
If so, it'd be cleaner to check the list instead of checking the result: consider what happens if a later version of Ruby adds a map
method to 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.
What about using Ruby goodness?
result = DeepL.translate(list, to_deepl_compatible_locale(from), to_deepl_compatible_locale(to), options)
[*result].map(&:text)
The splat operator would leave it all to Ruby ;-)
The current test in
|
DeepL return type is polymorphic and returns a single value instead of an array when given a single element array. Thanks to @lukasbischof, @jesusalc, @schmijos Fixes #363 Fixes #347
Fixed in d31297b |
It crashes when Deepl responds with a String. This fixes that.