Opened 11 years ago

Closed 11 years ago

#19160 closed New feature (fixed)

Make ungettext_lazy really usable

Reported by: Claude Paroz Owned by: nobody
Component: Internationalization Version: dev
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 Claude Paroz 11 years ago.
Proof of concept
19160-1.diff (6.0 KB ) - added by Claude Paroz 11 years ago.
Different implementation (+docs and tests)
19160.smart-ngettext-lazy.diff (5.0 KB ) - added by Julien Phalip 11 years ago.
19160.resolvable-ungettext.diff (6.7 KB ) - added by Alexey Boriskin 11 years ago.
Another implementation
19160-4.diff (6.8 KB ) - added by Claude Paroz 11 years ago.
Aggregated patch
19160-5.diff (7.5 KB ) - added by Alexey Boriskin 11 years ago.
Tiny fix of 19160-4.diff

Download all attachments as: .zip

Change History (34)

by Claude Paroz, 11 years ago

Attachment: 19160-poc.diff added

Proof of concept

comment:1 by Claude Paroz, 11 years ago

Has patch: set
Needs documentation: set
Needs tests: set

by Claude Paroz, 11 years ago

Attachment: 19160-1.diff added

Different implementation (+docs and tests)

comment:2 by Claude Paroz, 11 years ago

Needs documentation: unset
Needs tests: unset

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

comment:3 by Julien Phalip, 11 years ago

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

in reply to:  3 comment:4 by Claude Paroz, 11 years ago

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?

by Julien Phalip, 11 years ago

comment:5 by Julien Phalip, 11 years ago

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 by Claude Paroz, 11 years ago

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.

by Alexey Boriskin, 11 years ago

Another implementation

comment:7 by Alexey Boriskin, 11 years ago

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 by Alexey Boriskin, 11 years ago

Cc: sun.void@… added

comment:9 by Claude Paroz, 11 years ago

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 by Alexey Boriskin, 11 years ago

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

comment:11 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:12 by Aymeric Augustin, 11 years ago

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

by Claude Paroz, 11 years ago

Attachment: 19160-4.diff added

Aggregated patch

comment:13 by Claude Paroz, 11 years ago

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

by Alexey Boriskin, 11 years ago

Attachment: 19160-5.diff added

Tiny fix of 19160-4.diff

comment:14 by Alexey Boriskin, 11 years ago

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 by Aymeric Augustin, 11 years ago

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 by anonymous, 11 years ago

I asked Jacob on IRC: he doesn't remember a specific reason making it impossible to switch to single-underscore-prefixes in lazy. To be tested.

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:17 by Simon Charette, 11 years ago

Cc: charette.s@… added

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

comment:18 by Alexey Boriskin, 11 years ago

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 by Aymeric Augustin, 11 years ago

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 by Alexey Boriskin, 11 years ago

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 by Alexey Boriskin, 11 years ago

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

comment:22 by Aymeric Augustin, 11 years ago

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 by Claude Paroz, 11 years ago

Triage Stage: AcceptedReady 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 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 3f1a0c0040b9a950584f4b309a1b670b0e709de5:

Fixed #19160 -- Made lazy plural translations usable.

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

comment:25 by Alexey Boriskin, 11 years ago

Resolution: fixed
Status: closednew

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 by Aymeric Augustin, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

comment:27 by Alexey Boriskin, 11 years ago

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

comment:28 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: newclosed

In ace9d4efc3e6e0b07fd94b039d62e0d32c81ed3f:

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

Fixed #19160 (again). Thanks Alexey Boriskin.

Note: See TracTickets for help on using tickets.
Back to Top