wagtail-localize icon indicating copy to clipboard operation
wagtail-localize copied to clipboard

Support Deepl free api

Open vladox opened this issue 3 years ago • 2 comments

Essentially it enables the Deepl API to be changed since they have two different (free and pro). Also some improvements were made on error handling:

image

There's a new option to define the Deepl API endpoint:

WAGTAILLOCALIZE_MACHINE_TRANSLATOR = {
    "CLASS": "wagtail_localize.machine_translators.deepl.DeepLTranslator",
    "OPTIONS": {
        "AUTH_KEY": os.environ.get('DEEPL_API_KEY', None),
        "API_ENDPOINT": os.environ.get('DEEPL_API_ENDPOINT', None),
    },
}

vladox avatar Jan 18 '22 01:01 vladox

That doesn't seem to apply for AUTH_KEY and seems too verbose for an option. Since it can also be used in the future for other endpoints I'd leave it as it is.

On Tue, 15 Feb 2022 at 18:48, Dan Braghis @.***> wrote:

@.**** commented on this pull request.

In wagtail_localize/machine_translators/deepl.py https://github.com/wagtail/wagtail-localize/pull/505#discussion_r807134928 :

@@ -17,8 +27,9 @@ class DeepLTranslator(BaseMachineTranslator): display_name = "DeepL"

 def translate(self, source_locale, target_locale, strings):
  •    api_endpoint = self.options.get("API_ENDPOINT", "https://api.deepl.com/v2/translate")
    

nitpick: missed the first time around, but we do prefix our settings with WAGTAILLOCALIZE_ so this should be consistent. i.e. WAGTAILLOCALIZE_DEEPL_API_ENDPOINT

— Reply to this email directly, view it on GitHub https://github.com/wagtail/wagtail-localize/pull/505#pullrequestreview-883404415, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUGTDMVB76J7T7RQZIZH7DU3KGXJANCNFSM5MF5JKHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

vladox avatar Feb 16 '22 08:02 vladox

That doesn't seem to apply for AUTH_KEY and seems too verbose for an option. Since it can also be used in the future for other endpoints I'd leave it as it is.

@vladox sorry for the noise, I realised the options already came via WAGTAILLOCALIZE_MACHINE_TRANSLATOR after I made the comment, so removed it

zerolab avatar Feb 16 '22 14:02 zerolab

#604 provides a simpler approach to this, so may use that instead

zerolab avatar Aug 29 '22 11:08 zerolab

Went with #604. Thank you very much for this @vladox

zerolab avatar Sep 12 '22 09:09 zerolab

@zerolab thanks for the update but be aware that the implementation in #604 is missing the validation and error messages in case the URL doesn't match the key.

vladox avatar Sep 12 '22 10:09 vladox

@vladox #604 conditionally loads the endpoint based on the key (deepl library reference)

zerolab avatar Sep 12 '22 10:09 zerolab

@zerolab there's also a part of the PR that handles exceptions from the API and shows them correctly formatted to the end user, see class ApiEndpointError(Exception). I can do a separate PR only for that part since I think it's necessary to make the integration more user friendly.

vladox avatar Sep 23 '22 16:09 vladox

@vladod you're right, let's do it. Just make sure you mock the request/response

zerolab avatar Sep 23 '22 17:09 zerolab