Code

#19160 closed New feature (fixed)

Make ungettext_lazy really usable

Reported by: claudep Owned by: nobody
Component: Internationalization Version: master
Severity: Normal Keywords:
Cc: sun.void@…, charette.s@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In current Django code, ungettext_lazy is almost unusable, because you generally don't know the third argument (number) at lazy definition time. See #19158 and #19034 for use cases. It should be possible to pass the number argument at lazy resolution time.

Here's what we should be able to do:

class MyClass:
    # Note here the missing third argument
    string = ungettext_lazy("%(num)d string", "%(num)d strings")

    def resolve(self, number):
        # We hope here that the lazy 'magic' will be able to retrieve and pass number to ngettext
        return self.string % {'num': number}

Attachments (6)

19160-poc.diff (1.0 KB) - added by claudep 21 months ago.
Proof of concept
19160-1.diff (6.0 KB) - added by claudep 21 months ago.
Different implementation (+docs and tests)
19160.smart-ngettext-lazy.diff (5.0 KB) - added by julien 21 months ago.
19160.resolvable-ungettext.diff (6.7 KB) - added by void 21 months ago.
Another implementation
19160-4.diff (6.8 KB) - added by claudep 19 months ago.
Aggregated patch
19160-5.diff (7.5 KB) - added by void 19 months ago.
Tiny fix of 19160-4.diff

Download all attachments as: .zip

Change History (34)

Changed 21 months ago by claudep

Proof of concept

comment:1 Changed 21 months ago by claudep

  • Has patch set
  • Needs documentation set
  • Needs tests set

Changed 21 months ago by claudep

Different implementation (+docs and tests)

comment:2 Changed 21 months ago by claudep

  • Needs documentation unset
  • Needs tests unset

I've just uploaded a different implementation, a bit less hacky, including tests and documentation.

comment:3 follow-up: Changed 21 months ago by julien

Looking at the patch, I think it makes sense. I'll spend some time to do a deeper review, but for now I just had one question: can we allow to have anonymous parameters? e.g.:

class MyClass:

    string = ungettext_lazy("%d string", "%d strings")

    def resolve(self, number):
        return self.string % number

comment:4 in reply to: ↑ 3 Changed 21 months ago by claudep

Replying to julien:

Looking at the patch, I think it makes sense. I'll spend some time to do a deeper review, but for now I just had one question: can we allow to have anonymous parameters? e.g.:

No, this wouldn't work. I'm a bit reluctant to complicate this too much, I'd rather augment the documentation with limitations. But if you find some elegant way to achieve this, why not?

Changed 21 months ago by julien

comment:5 Changed 21 months ago by julien

I've attached a patch with a different approach to allow it to work both with named and anonymous parameters. It doesn't modify functional.py but it does monkey-patches the proxy's __mod__ descriptor, which I concede is a bit of a code smell. Thoughts?

comment:6 Changed 21 months ago by claudep

It's nice that it's working with anonymous parameter, but this will unfortunately not work properly when the string has more than one placeholder, like in "Hello %(name)s, you have %(num)d new messages". The .values()[0] part will arbitrarily pick one dictionary value, and in the case of unnamed parameters, you will pass the tuple to ungettext.

Changed 21 months ago by void

Another implementation

comment:7 Changed 21 months ago by void

I've uploaded another implemtation, what do you think about it? There may be a confusing moment about when to call .resolve() and when not, and it also modifies the thing I probably should not be messed with - lazy()

comment:8 Changed 21 months ago by void

  • Cc sun.void@… added

comment:9 Changed 21 months ago by claudep

There are interesting parts in your implementation. However, the fact that you have to call a method (resolve()) is creating a special case for translatable strings and defeats the ability to swap a ungettext_lazy object with any other string.

Basically, the use case of lazy gettext strings is to have a string defined at class/module level (defined at compilation time), and then use that string somewhere later at runtime. So it should be totally acceptable to have the string redefined somewhere in the code. Therefore, you don't know for sure at runtime if the string is an ungettext_lazy or any other string object. So having to call a special method on the string is a no-go for me.

comment:10 Changed 21 months ago by void

Patch without resolve() method: https://gist.github.com/3945679

comment:11 Changed 21 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:12 Changed 19 months ago by aaugustin

Reviewing the discussion and the patches above, I think Claude nailed the API in 19160-1.diff​ and Julien the implementation in 19160.smart-ngettext-lazy.diff​.

I suggest to adapt Julien's patch to Claude's API.

For anonymous parameters, I recommend to support only a single integer parameter. If no number argument is passed to ngettext_lazy, and later on the result is interpolated with a single integer value, that value should be used as the "count".

Changed 19 months ago by claudep

Aggregated patch

comment:13 Changed 19 months ago by claudep

I mixed Julien's version with mine, mind to review?

Changed 19 months ago by void

Tiny fix of 19160-4.diff

comment:14 Changed 19 months ago by void

I like the patch 19160-4.diff except the line key=number, which does nothing but renames global variable for use in the function which has local variable with the same name. Attached patch uses another name for the local variable and no rename is needed thereby

comment:15 Changed 19 months ago by aaugustin

I reviewed the patch.


To sum up, the concept of *_lazy functions is to return a proxy object that stores the original function and its argument. When the proxy object is converted to a string, the function is evaluated. It can return different results if it takes into account a global (usually thread-local) variable — hire, the active language.

The complication here is that the number argument isn't known when the proxy object is built. We don't want to add an API to insert it (as acknowledged in comment 7). We want to extract it automatically when string interpolation is used on the proxy object (ie. the % operator, that invokes __mod__).

It would still be easy to inject the extra keyword argument, if keyword arguments weren't a "private" attribute of the proxy class (__proxy__.__kw)! "Private" attributes aren't actually private in Python; we can still set them with the _<classname>_<attrname> hack. Ugly — that's what we get for bending django.utils.functional.lazy.

(By the way, injecting a keyword argument is better than injecting a positional argument, because it works regardless of whether the previous arguments were positional or keyword arguments.)

The current implementation of lazy dates back to the merge of the i18n branch, more than 7 years ago. Maybe we could switch from double-underscore-prefixes to single-underscore-prefixes, alleviating the need for the _<classname>_<attrname> hack?

That won't change fundamentally the implementation — we're still hacking the proxy object's internals — but at least that'll avoid using a not-so-well-known Python hack.


There's a little bit of work on the patch, but it can be done by the committer:

  • In the docs: .. versionadded:: 1.5 => .. versionchanged:: 1.6.
  • The docs could use a little bit of copy-editing.
  • In the tests: s = ungettext_lazy("%d good result", "%d good result") => the second arg should be a plural.

Last minute thought: if the ngettext function gets called with number=None — for instance because the lazy object is resolved without interpolation — is it easy to understand the traceback? We're explicitly introducing a default value of number=None here, possibly triggering complicated errors when the developer doesn't use one of the three supported syntaxes.

comment:16 Changed 19 months ago by anonymous

I asked Jacob on IRC: he thinks it's possible to switch to single-underscore-prefixes in lazy.

Version 0, edited 19 months ago by anonymous (next)

comment:17 Changed 18 months ago by charettes

  • Cc charette.s@… added

Do we need to test the single-underscore-prefixes lazy related switch before committing this?

comment:18 Changed 18 months ago by void

Double underscores are there for a reason. If we'd switch to single underscore and use lazy() on function with the resultclass, which has clashing single underscore attribute, that attribute will be overwritten, which is no good.

comment:19 Changed 18 months ago by aaugustin

We talked a lot about this ticket on IRC. The latest discussion concluded with:

2013-01-03 23:41:27 <void> mYk: I wonder if there is a way to rewrite lazy() with __get__/__set__ descriptors instead of all that copying
2013-01-03 23:43:10 <mYk> it would probably be cleaner and faster...
2013-01-03 23:45:24 <void> mYk: yes. but I am not sure if there are some hidden pitfalls that way

This could help avoid the _<classname>_<attrname> hack.

comment:20 Changed 18 months ago by void

Another approach to solve this: https://gist.github.com/4605085 (To use this, we need part of #19642 about __mod__ solved, patch is at https://gist.github.com/4605065)

comment:21 Changed 18 months ago by void

I've sent a pull request with code cleaned up a bit: https://github.com/django/django/pull/684

comment:22 Changed 18 months ago by aaugustin

This looks way better than the previous attempts. It's probably RFC — it just needs running the tests on Python 2 and 3 and proof-reading carefully.

comment:23 Changed 18 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Thanks Alexey! Tests have been successfully run on Python 2.6/SQLite and Python 3.2/SQLite.

To prevent deprecation warnings, self.assertRaisesRegexp should be changed to six.assertRaisesRegex. Apart from that, it looks very nice.

comment:24 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 3f1a0c0040b9a950584f4b309a1b670b0e709de5:

Fixed #19160 -- Made lazy plural translations usable.

Many thanks to Alexey Boriskin, Claude Paroz and Julien Phalip.

comment:25 Changed 18 months ago by void

  • Resolution fixed deleted
  • Status changed from closed to new

Our new ungettext_lazy function lacks support for pretty trivial, but widespread usecase, when we don't have number in a string we're translating.
For example, I tried to replace lambda-hack in django.utils.timesince (#19704), and it was rather confusing to use ungettext_lazy('year', 'years', 'n') % {'n': count} instead of just ungettext_lazy('year', 'years') % count

comment:26 Changed 18 months ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

I suggest to catch TypeError, and if it's raised, retry without interpolation.

comment:27 Changed 18 months ago by void

https://github.com/django/django/pull/692 - pull request implementing that try/catch for TypeError

comment:28 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In ace9d4efc3e6e0b07fd94b039d62e0d32c81ed3f:

Made ungettext_lazy usable for messages that do not contain the count.

Fixed #19160 (again). Thanks Alexey Boriskin.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.