search

FlineDev / BartyCrouch

Localization/I18n: Incrementally update/translate your Strings files from .swift, .h, .m(m), .storyboard or .xib files.
MIT License
1.37k stars 121 forks source link

Fix that DeepL translation doesn't work #269

Closed mshibanami closed 2 years ago

mshibanami commented 2 years ago

(No related issues.)

Proposed Changes

Currently, BartyCrouch always fails to call DeepL API's URL (https://api-free.deepl.com/v2/translate). 400 error is returned and the response body includes this:

{
    "message": "Invalid JSON request."
}

This seems to happen because Content-Type: application/json is specified. Since this is a GET request, I think we shouldn't include it. So I just deleted it.

Other

According to https://www.deepl.com/en/docs-api, BartyCrouch should actually call this URL as a POST request. It even works as a GET request as of now, but I don't know why. Historical reasons, maybe? Sorry, I don't have enough time to work 🙂, but I suggest following that documentation so that no future problems.

Jeehut commented 2 years ago

@mshibanami Thank you for bringing this up and trying to fix it.

I had not implemented the DeepL support myself, it was added by the community, so I can't say why a GET request was implemented. But as part of my RemafoX app implementation, I've read the docs and it seems that the expected Content-Type is application/x-www-form-urlencoded, see this screenshot (in case the docs change):

Screenshot 2022-11-24 at 19 56 57

So, your PR isn't quite right. And as you also mentioned, we were making a GET request here, but the API clearly states we should make a POST request. My best guess is that it was specified as a GET at first, but then the API authors at DeepL ran into URL query length limitations, so they've moved the URL parameters to the body within a POST request (which the content-type clearly indicates). So, the API might have actually changed over time.

I've just copied over the relevant portion of my implementation from RemafoX to BartyCrouch, so this PR should no longer be needed, it's covered in 459e936d7a95e05c4793ff3b8a02ea925e6fe8a8.

Thank you again for taking the time to report and try to fix this. It's much appreciated and led to me investing some time to write the commit. :)

Greetings from Nikko!

mshibanami commented 2 years ago

@Jeehut Ah, cool. Yes, that IS the right way to solve this issue. Mine was just a hotfix. Thanks!

Greetings from Nikko!

Oh, so you're still in Japan. Have fun with monkeys and Jidai village in Nikko! 😄