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

Support Deepl free api #505

Closed
wants to merge 5 commits into from

Conversation

vladox
Copy link

@vladox vladox commented Jan 18, 2022

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),
    },
}

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Thanks you @vladox!
Did a quick first pass review with a minor suggestion.
The main thing here is we need a couple of tests (one for the setting and the other for the error message)

@vladox vladox requested a review from zerolab February 16, 2022 08:34
@vladox
Copy link
Author

vladox commented Feb 16, 2022 via email

@zerolab
Copy link
Collaborator

zerolab commented Feb 16, 2022

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

}
)
def test_deepl_machine_translate_page(self):
response = self.client.post(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but this will actually make and API request to the DEEPL endpoint. We should avoid this and instead mock the response

@zerolab
Copy link
Collaborator

zerolab commented Aug 29, 2022

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

@zerolab
Copy link
Collaborator

zerolab commented Sep 12, 2022

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

@zerolab zerolab closed this Sep 12, 2022
@vladox
Copy link
Author

vladox commented Sep 12, 2022

@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.

@zerolab
Copy link
Collaborator

zerolab commented Sep 12, 2022

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

@vladox
Copy link
Author

vladox commented Sep 23, 2022

@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.

@zerolab
Copy link
Collaborator

zerolab commented Sep 23, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants