Code review comment for lp:~jr/bzr/i18n-errors

Revision history for this message
Martin Packman (gz) wrote :

Summarizing from IRC conversion, the error module has a lot of pre-existing problems that have been mostly safe to ignore because:

* All _fmt strings are ascii str, which can be upcast to either non-ascii bytes or unicode (but still break when trying to do both, see bug 273978 for instance).
* Many users have utf-8 consoles so printing utf-8 encoded error messages rather than encoding unicode appropriately appears fine.

In making the _fmt string a localised utf-8 message, these existing confusions with string types are exposed.

+ unicode_fmt = osutils.safe_unicode(fmt)
+ if type(fmt) == unicode:
+ trace.mutter("Unicode strings in error.fmt are deprecated")

I'd replace this with just unicode(fmt) and add a comment that all _fmt strings should be ascii.

+ return gettext(unicode_fmt).encode('utf-8')

Remove the encode here. That will fix exceptions that include paths, or any other unicode strings in the message. On the other hand, it breaks exceptions that think it's safe to interpolate arbitrary bytes into exception messages, but they're already bogus and need fixing anyway.

+ self.overrideAttr(i18n, '_translations', ZzzTranslations())
+ try:
+ workingtree.WorkingTree.open('./foo')
+ except errors.NotBranchError,e:
+ err = str(e)
+ self.assertContainsRe(err, "zz{{Not a branch: .*}}")

If we can make ZzzTranslations return more than just ascii it would be more realistic, and this test would then fail with a UnicodeError.

review: Needs Fixing

« Back to merge proposal