#1606 Add Deepl translator module #1671
Conversation
| private val deeplClient = DeeplClient(property ("tock_translator_deepl_api_url", "default"),property ("tock_translator_deepl_api_key", "default")) | ||
| private val glossaryId = property ("tock_translator_deepl_glossaryId", "default") |
There was a problem hiding this comment.
These properties should be documented in the module's readme
There was a problem hiding this comment.
Also does it even make sense to provide default values here? I believe this module should error ASAP when enabled without valid settings.
translator/deepl-translate/src/main/kotlin/DeeplTranslatorEngine.kt
Outdated
Show resolved
Hide resolved
translator/deepl-translate/src/main/kotlin/DeeplTranslatorIoc.kt
Outdated
Show resolved
Hide resolved
| /* | ||
| * Copyright (C) 2017/2021 e-voyageurs technologies | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
This copyright notice should be above the package declaration I think. Speaking of which, there is no package declaration in this file...?
| */ | ||
| class DeeplTranslateIntegrationTest { | ||
| @Test | ||
| @Disabled |
There was a problem hiding this comment.
If every method is disabled like this, maybe it would be worth just disabling the entire class? Ideally though, there should be functional end-to-end tests with wiremock to simulate Deepl's responses.
| @Disabled | ||
| fun testWithHTML() { | ||
| val result = DeeplTranslatorEngine.translate( | ||
| "Bonjour, je voudrais me rendre à Paris <br><br/> demain soir", |
There was a problem hiding this comment.
How does Deepl behave with a self-closing <br/> tag? I believe that would be the most common form, if it occurred. Could also try more advanced HTML structures, like a list (somewhat common in web connector messages)
| private val apiKey: String? = propertyOrNull("tock_translator_deepl_api_key"), | ||
| okHttpCustomizer: OkHttpClient.Builder.() -> Unit = {} | ||
| ) : DeeplClient { | ||
| private val client = OkHttpClient.Builder().apply(okHttpCustomizer).build() |
There was a problem hiding this comment.
This client should use the TockProxyAuthenticator by default
| import com.github.salomonbrys.kodein.bind | ||
| import com.github.salomonbrys.kodein.provider | ||
|
|
||
| fun deeplTranslatorModule(client: DeeplClient = OkHttpDeeplClient()) = Kodein.Module { |
There was a problem hiding this comment.
I think we should have a default val deeplTranslatorModule = deeplTranslatorModule() for consistency with other modules
| private val supportedLanguagesProperty = propertyOrNull("tock_translator_deepl_target_languages") | ||
| private val supportedLanguages: Set<String>? = supportedLanguagesProperty?.split(",")?.map { it.trim() }?.toSet() |
There was a problem hiding this comment.
Why not inline supportedLanguagesProperty?
| override fun translate(text: String, source: Locale, target: Locale): String { | ||
| var translatedTextHTML4 = "" | ||
| // Allows to filter translation on a specific language | ||
| if (supportedLanguages?.contains(target.language) == true || supportedLanguages == null) { |
There was a problem hiding this comment.
You could swap the two conditions to benefit from the smart cast
|
|
||
| val request = Request.Builder() | ||
| .url(apiURL) | ||
| .addHeader("Authorization", "DeepL-Auth-Key $apiKey") |
There was a problem hiding this comment.
apiKey could be null at this point in the code. The client should fail way earlier if that is the case.
…glossary map ids for all languages
| .build() | ||
|
|
||
| glossaryId?.let { | ||
| formBuilder.add("glossaryId", it) |
There was a problem hiding this comment.
| formBuilder.add("glossaryId", it) | |
| formBuilder.add("glossary_id", it) |
Based on https://developers.deepl.com/docs/api-reference/translate/openapi-spec-for-text-translation
theopenconversationkit#1606 Add Deepl translator module - second set of corrections
* #1606 Add Deepl translator module * #1606 Add Deepl translator module - first set of corrections * #1606 Add Deepl translator module - first set of corrections * #1606 Add Deepl translator module - second set of corrections * #1606 : Use of TockProxyAuthenticator + use of glossary map ids for all languages * #1606 translator: clean up module * #1606 : Correction of the glossary id name --------- Co-authored-by: charles_moulhaud <[email protected]> Co-authored-by: Fabilin <[email protected]>
No description provided.