Opened 12 years ago
Closed 12 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)
Change History (34)
by , 12 years ago
Attachment: | 19160-poc.diff added |
---|
comment:1 by , 12 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
comment:2 by , 12 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
I've just uploaded a different implementation, a bit less hacky, including tests and documentation.
follow-up: 4 comment:3 by , 12 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
comment:4 by , 12 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 , 12 years ago
Attachment: | 19160.smart-ngettext-lazy.diff added |
---|
comment:5 by , 12 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 , 12 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.
comment:7 by , 12 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 , 12 years ago
Cc: | added |
---|
comment:9 by , 12 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:11 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:12 by , 12 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".
comment:14 by , 12 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 , 12 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 , 12 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.
comment:17 by , 12 years ago
Cc: | added |
---|
Do we need to test the single-underscore-prefixes lazy
related switch before committing this?
comment:18 by , 12 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 , 12 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 , 12 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 , 12 years ago
I've sent a pull request with code cleaned up a bit: https://github.com/django/django/pull/684
comment:22 by , 12 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 , 12 years ago
Triage Stage: | Accepted → 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 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:25 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 12 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I suggest to catch TypeError
, and if it's raised, retry without interpolation.
comment:27 by , 12 years ago
https://github.com/django/django/pull/692 - pull request implementing that try/catch for TypeError
comment:28 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Proof of concept