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

Translation missing for part of throttle warning #6177

Closed
GitRon opened this issue Sep 11, 2018 · 7 comments · Fixed by #6771
Closed

Translation missing for part of throttle warning #6177

GitRon opened this issue Sep 11, 2018 · 7 comments · Fixed by #6771
Milestone

Comments

@GitRon
Copy link

GitRon commented Sep 11, 2018

I'm using the throttling mechanism of DRF. When the exception is shown that actually the request was throttled, one part of the message is translated, the other part is not:

grafik

I couldn't find the part "expected available in X seconds" in the translation file so I guess it is just missing?

I'm using djangorestframework==3.8.2.

Best regards
Ron

@rpkilby
Copy link
Member

rpkilby commented Sep 11, 2018

It looks like the Throttled exception doesn't have translations for the extra detail.

class Throttled(APIException):
status_code = status.HTTP_429_TOO_MANY_REQUESTS
default_detail = _('Request was throttled.')
extra_detail_singular = 'Expected available in {wait} second.'
extra_detail_plural = 'Expected available in {wait} seconds.'
default_code = 'throttled'

@dennishylau
Copy link

Any quick workaround possible?

@rpkilby
Copy link
Member

rpkilby commented Dec 8, 2018

Re-raise as a custom Exception class that has translations?

@cristianocca
Copy link

cristianocca commented Jun 18, 2019

Bump. Just faced this, is it possible to get this fixed in the next minor release?

Rather than a missing translation, this is also looks like a misuse of the translation framework.

Monkey patch the exception in the meantime.

# Adding this here so we force the translation to happen
# because DRF is bugged, monkey patch it.
# Remove once https://github.com/encode/django-rest-framework/issues/6177 is fixed
from django.utils.translation import gettext_lazy as _
from rest_framework.exceptions import Throttled
Throttled.extra_detail_singular = _("Expected available in {wait} second.")
Throttled.extra_detail_plural = _("Expected available in {wait} seconds.")

@rpkilby rpkilby added this to the 3.10 Release milestone Jun 18, 2019
@rpkilby
Copy link
Member

rpkilby commented Jun 18, 2019

Adding this to the milestone, as I don't think there is any significant complexity in fixing this.

Rather than a missing translation, this is also looks like a misuse of the translation framework.

Can you clarify? ngettext is still used to switch between the two messages.

@cristianocca
Copy link

cristianocca commented Jun 18, 2019

Adding this to the milestone, as I don't think there is any significant complexity in fixing this.

Rather than a missing translation, this is also looks like a misuse of the translation framework.

Can you clarify? ngettext is still used to switch between the two messages.

I'm not 100% sure about what's the issue, but ngettext doesn't seem to be working unless the text itself is also a translated string. Please see my monkey patch above.

Looks like this line:

force_text(ungettext(self.extra_detail_singular.format(wait=wait),
                                     self.extra_detail_plural.format(wait=wait),
                                     wait))))

Will only work if extra_detail_singular and extra_detail_plural are lazy translation strings (or translated in place).
Perhaps it is my Windows environment and gettext, but ungettext wasn't giving me translated text even if I added the translation string by hand.

Edit: Probably related to the fact that a variable is used there, and that .format is also used. Please read the django docs for a good example about how to use ungettext: https://docs.djangoproject.com/en/2.2/topics/i18n/translation/#pluralization

@rpkilby
Copy link
Member

rpkilby commented Jun 18, 2019

Probably related to the fact that a variable is used there, and that .format is also used.

This would make sense.

Please read the django docs for a good example about how to use ungettext

DRF uses .format instead of %-based formatting, so we can't just pass the strings directly to the various gettext functions. That said, wrapping the format strings in gettext_lazy should fix the issue.

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 a pull request may close this issue.

4 participants