Recursive Translations of Message strings with mappings

Reported by Hermann Himmelbauer on 2008-04-01
4
Affects Status Importance Assigned to Milestone
BlueBream
Undecided
Unassigned
Zope 3
Wishlist
Christian Zagrodnick

Bug Description

The following is not really a bug, but a missing feature. I need to translate messages that contain mappings with messages. Zope3 currently only translates the toplevel message and leaves the mappings untranslated, for instance:

--------------- snip ------------------
from zope.component import provideUtility
from zope.i18n import translate
from zope.i18n.simpletranslationdomain import SimpleTranslationDomain
from zope.i18nmessageid import Message, MessageFactory
_ = MessageFactory('xyz')

messages = {
    ('en', u'pink'): u'pink',
    ('de', u'pink'): u'rosa',
    ('en', u'colour'): u'The colour is $pink',
    ('de', u'colour'): u'Die Farbe ist $pink'}

xyz = SimpleTranslationDomain('xyz', messages)
provideUtility(xyz, name=u'xyz')

pink = _(u'pink', u'pink')
pink = Message(pink)
print translate(pink, target_language='en')
print translate(pink, target_language='de')

colour = _(u'colour', u'The colour is $pink')
colour = Message(colour, mapping={'pink' : pink})
print translate(colour, target_language='en')
print translate(colour, target_language='de')
------------ snip -----------------

The output of the above program is:

------
pink
rosa
The colour is pink
Die Farbe ist pink
------

It can be seen that the string "pink" is not translated if used as mapping in
another string to be translated.

The problem is easily solved by modifying the translate function in i18n.translationdomain.TranslationDomain (line 84):

    def translate(self, msgid, mapping=None, context=None,
                  target_language=None, default=None):
        """See zope.i18n.interfaces.ITranslationDomain"""
.....
        # MessageID attributes override arguments
        if isinstance(msgid, Message):
            if msgid.domain != self.domain:
                util = getUtility(ITranslationDomain, msgid.domain)
            mapping = msgid.mapping
            default = msgid.default
            # FIX BEGIN
            # Recursively translate mappings, if they are translatable
            if isinstance(mapping, dict):
                for key, value in mapping.items():
                    if isinstance(value, Message):
                        mapping[key]=self.translate(value, mapping, context,
                                                    target_language, default)
            # FIX END

        if default is None:
            default = unicode(msgid)

The fix works perfectly for me, however, I'm not sure if it would break other things in Zope3, moreover one should probably write quick test for it.

The new diff looks much better. Thanks Hermann! I only have three more
comments :)

* I think we need a few more tests, especially for the edge cases. For
instance, what happens when you do:

   msg1 = _('Message 1 and $msg2', mapping={})
   msg1.mapping['msg2'] = _('Message 2 and $msg1', mapping={'msg1':
msg1})

I know, it's a bit of an edge case, but it should be dealt with.

* Copying the dictionary shouldn't be done with copy.copy. It can
easily be done with mapping.copy(). In other words, dictionaries have
a copy() method, you should use it.

* Style: Wrapping a long if-line is more nicely wrapped using
parantheses, rather than backspace. I suppose it's a matter of taste,
but PEP8 suggests using parantheses ("The preferred way of wrapping
long lines is by using Python's implied line continuation inside
parentheses, brackets and braces.").

Ok, I added the above test with circular references, I raise a RuntimeError in this case (could not think of a better exception).

Moreover I now do a mapping.copy() and use parantheses for wrapping long lines.

What I still think of is if this recursive translation should also be included in the
__init__.translate() function and also in the SimpleTranslationdomain.translate() function.

What do you think?

On 2 Apr 2008, at 13:06 , Hermann Himmelbauer wrote:
> Ok, I added the above test with circular references, I raise a
> RuntimeError in this case (could not think of a better exception).

I suggest a ValueError instead.

> Moreover I now do a mapping.copy() and use parantheses for wrapping
> long
> lines.

Thanks, but the wrapping isn't exactly done how it's supposed to be.
This is the correct form:

   if (mapping is not None and
       Message in [type(m) for m in mapping.values()]):

I'm not very happy with the newest patch for other reasons, however.
You changed the signature of the translate() method. You can't do
that, it's governed by the ITranslationDomain interface. Public
interfaces like that must not change.

Stylistically, I have two things to nitpick about:

* Comparisons to None should be made with the 'is' operator (see PEP8)

* The way the 'to_translate' variable is used, a 'set' object would be
more suitable than a list.

Christian Zagrodnick (zagy) wrote :

Hey,

any update on this? Since I'd need this rather soon I'd also fix the outstanding issues and check it in.

Objections?

Changed in zope3:
importance: Undecided → Wishlist

On 18 Apr 2008, at 16:34 , Christian Zagrodnick wrote:
> any update on this? Since I'd need this rather soon I'd also fix the
> outstanding issues and check it in.
>
> Objections?

Not from me!

Am Freitag, 18. April 2008 16:34 schrieb Christian Zagrodnick:
> Hey,
>
> any update on this? Since I'd need this rather soon I'd also fix the
> outstanding issues and check it in.
>
> Objections?
>
> ** Changed in: zope3
> Importance: Undecided => Wishlist

Well, I'd like to fix this but I'm still unsure about the interface problem
(extra method attribute). What's your opinion on this?

Best Regards,
Hermann

--
<email address hidden>
GPG key ID: 299893C7 (on keyservers)
FP: 0124 2584 8809 EF2A DBF9 4902 64B4 D16B 2998 93C7

Christian Zagrodnick (zagy) wrote :

fixed in r85508

Not backporting because its a feature, right?

Changed in zope3:
assignee: nobody → zagy
status: New → Fix Committed

On 20 Apr 2008, at 19:23 , Christian Zagrodnick wrote:
> fixed in r85508

with quite a lot of XXX comments in it... Can you resolve those please?

> Not backporting because its a feature, right?

Right.

Thanks!

Christian Zagrodnick (zagy) wrote :
  • smime.p7s Edit (4.2 KiB, application/pkcs7-signature; name=smime.p7s)

On 20.04.2008, at 21:33, Philipp von Weitershausen wrote:
> On 20 Apr 2008, at 19:23 , Christian Zagrodnick wrote:
>> fixed in r85508
>
> with quite a lot of XXX comments in it... Can you resolve those
> please?

Gah. I at least forgot one of them (RuntimeError/ValueError).

I'll create an issue for the other (Message interface).

--
Christian Zagrodnick

gocept gmbh & co. kg · forsterstrasse 29 · 06112 halle/saale
www.gocept.com · fon. +49 345 12298894 · fax. +49 345 12298891

Christian Zagrodnick (zagy) wrote :

There is an open issue, so back to in progress: The current implementation doesn't work if the messages in the mapping are in a different translation domain.

Changed in zope3:
status: Fix Committed → In Progress
Brian Sutherland (jinty) wrote :

Er, I seem to have fixed this bug without checking the bugtracker first:

http://mail.zope.org/pipermail/checkins/2009-April/033287.html
http://mail.zope.org/pipermail/checkins/2009-April/033288.html

I'd appreciate a review if anyone has the time.

Changed in zope3:
status: In Progress → Fix Committed
Tres Seaver (tseaver) on 2010-04-15
Changed in zope3:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments