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

Deepl API provider #369

Merged
merged 12 commits into from
Apr 14, 2024
Merged

Deepl API provider #369

merged 12 commits into from
Apr 14, 2024

Conversation

rafaelmardojai
Copy link
Member

@rafaelmardojai rafaelmardojai commented Jan 22, 2024

A correct initial implementation needed dome tweaks on other parts of Dialect's code.

There are some open questions:

  • Do we want to support pro users? DeepL has separate API domains for pro and free users (api-free.deepl.com and api.deepl.com). We could support this with our current "instances" feature, but that will require users manually setting the pro domain.

    A more complex solution could be made introducing some kind of limited "provider config fields" API so providers can define some configuration fields in the form of entries, dropdows, switches, etc, this also is sort of required if we want to implement an official Microsoft provider (see Add Microsoft provider #314).

    No plan to support api.deepl.com for now.

  • Do we know the char limit? The docs mention an 128 KiB limit, can we convert that to an approximate char limit?
    Set to 5000 characters.

TODO:

  • We need to extend the providers API to allow different src/dest lang lists. DeepL has two separated lists with some divergences.
  • Extend providers API to allow providers report API usage and limits. Then support the new API from the preferences window.

@mufeedali
Copy link
Member

Do we want to support pro users?

No tbh. I don't think that's needed. 500,000 characters per month should be enough for most users. That's over 16500 characters per day. Should be enough. If not... I don't think Dialect is really meant for that anyway...

Do we know the char limit?

Free API has a limit of 500,000 characters per month. Regarding the character limit per query, I think it's better if we enforce a limit from our end of maybe 2000 characters. 2000 characters would mean that it takes a user at least 8 queries to exceed their daily API limit (not really a limit, but the 16500 characters I mentioned earlier) and will definitely be under 128KBs as well.

DeepL has two separated lists with some divergences.

Between Pro and Free? I'd say skip pro. I feel like DeepL is being generous enough for most users.

Extend providers API to allow providers report API usage and limits.

Yes, we definitely need to add both UI and API support for that.

@rafaelmardojai
Copy link
Member Author

No tbh. I don't think that's needed. 500,000 characters per month should be enough for most users. That's over 16500 characters per day. Should be enough. If not... I don't think Dialect is really meant for that anyway...

I guess someone could "hack" Dialect and change the instance_url on GSettings.

Between Pro and Free? I'd say skip pro. I feel like DeepL is being generous enough for most users.

Nope, they have distinct langs list, for example in src they have English (en) and in dest they have both American (en_US) and British (en_GB), same for Portuguese.

I had some workaround code for that in my webkit-based implementation that I can copy, but probably would be better to rework how we handle langs to support this use case, also checking what would we need for offline providers.

@mufeedali
Copy link
Member

I guess someone could "hack" Dialect and change the instance_url on GSettings.

My thought process is just that anyone who needs the Pro probably doesn't need Dialect at all.

Nope, they have distinct langs list

Oh... Thats different from what I imagined. Then yeah, I guess we need a clean implementation for that.

@rafaelmardojai
Copy link
Member Author

My thought process is just that anyone who needs the Pro probably doesn't need Dialect at all.

Yep, agree.

@rafaelmardojai
Copy link
Member Author

API usage implemented:
Captura desde 2024-04-08 14-27-49-obfuscated

Convert all methods to instance methods since there's not real need for
static methods here, and some of them need to be overwritten by
subclasses.

Also pass the status code to check_known_errors, status errors are a
primary method for error handing.
@rafaelmardojai rafaelmardojai marked this pull request as ready for review April 10, 2024 01:59
@rafaelmardojai rafaelmardojai linked an issue Apr 10, 2024 that may be closed by this pull request
- Implement a ProviderSettings helper class to separate settings
implementation details
- Use libsecret sync methods to avoid complicating our API
@mufeedali
Copy link
Member

Found some bugs and fixed them. Should be good now :) Merging.

@mufeedali mufeedali merged commit b490323 into main Apr 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DeepL backend
2 participants