Re: [Freeipa-devel] [PATCH] jderose 052 Finish deferred translation mechanism
On Fri, 2010-03-12 at 11:31 -0500, John Dennis wrote:
> On 03/08/2010 11:25 PM, Jason Gerard DeRose wrote:
> > This patch finishes the the LazyText functionality in the ipalib.text
> > module. This patch includes extensive docstrings in text.py that should
> > hopefully explain everything pretty well. There's also now pretty darn
> > complete test coverage. Still to do:
> >
> >1. Have Backend.session extract the locale and set
> > context.languages... I have an rpcserver cleanup patch I've been
> > working on which will include this change.
> >
> >2. Remove deprecated gettext stuff in ipalib.request... this is a
> > small change, but I left it out of this patch so it's easier to
> > review
> >
> > I'll have these next two patches later this week.
>
> I've tested this and it works for me and seems pretty clean, a good
> patch. Thank you Jason. However I do have one thing which I'd like to
> see cleaned up, it's a few naming issues (see below).
Well, naming issues aside, is this an ack? Do you mind if I push this
patch and then possibly push a tune-up patch?
> In a moment I'm going to follow up with a patch that extends
> tests/test_ipalib/test_text.py to utilize the test language you asked
> for and is currently in install/po. That test is implemented and working
> so look for the patch in a moment.
>
> Naming Issues:
>
> The thread local object can be assigned attributes directly and it's
> attributes can be referenced directly. Using context.__dict__ seems odd
Although it isn't usually standard to use an instance dictionary like
this, the Python threading.local documentation specifically endorses it.
After reading the docstring in /usr/lib64/python2.6/_threading_local.py,
my impression is that threading.local is indented to be used both as an
instance to store thread-local attributes, and as a dict to store
thread-local items (regardless of whether the keys are valid attribute
names).
John, could you take a look at this documentation and let me know if you
concur?
> and unnecessary to store the language keys. I presume you're doing that
> because you can't have a tuple as an attribute name on the context.
> Directly accessing the __dict__ of an object feels like something we
> should avoid if possible. Also we're stuffing unrelated items in
> context.__dict__, for example the Connection and language keys are being
> stored together. Wouldn't be cleaner to keep the language keys in their
> own "name space" and to use constructs like this:
>
> context = threading.local()
> context.connection = Connection()
> context.language_keys = {}
> context.language_keys[key] = translation
> if key in context.language_keys
As you have it above, context.language_keys only exists in the current
thread. So each time we would have to check if the language_keys dict
has been created in the current thread, then check if the key is
present.
If you want these separated, I personally think a second threading.local
instance should be used, something like:
language_keys = threading.local()
I actually had them separated like this initially but decided to combine
them so there is only one threading.local instance we need to clear()
after processing a request.
Also, though it seems messy to combine all of these in the context, the
name-spaces don't overlap... a tuple will never equal an attribute name
(str), so the translations can't conflict with any attributes we store
on the context.
> rather than
>
> context.__dict__[key] = translation
> if key in context.__dict__
>
> This also means when you clear the context you don't have to iterate
> over the members of context.__dict__ and special case the values as is
> currently being done with:
>
> for (name, value) in context.__dict__.items():
> if isinstance(value, Connection):
> value.disconnect()
>
> Wouldn't this be cleaner as:
>
> if context.connection:
> context.connection.disconnect()
We can have multiple connections, which is why we do this iteration with
type checking. An LDAP connection is always created, but other
connections might also be created. Currently the only place we are
doing this is for a connection to the certificate server, but we should
allow plugins to create additional connections, and have them explicitly
disconnected by request.destroy_context().
> Keeping the language keys separately would also allow us to clear the
> language keys independently of anything else in the context without
> having to worry about what else we might clobber in the context.
I have no problem using a separate threading.local() instance for the
translations if you feel that is the better approach. Small change.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] jderose 052 Finish deferred translation mechanism
On 03/08/2010 11:25 PM, Jason Gerard DeRose wrote:
This patch finishes the the LazyText functionality in the ipalib.text
module. This patch includes extensive docstrings in text.py that should
hopefully explain everything pretty well. There's also now pretty darn
complete test coverage. Still to do:
1. Have Backend.session extract the locale and set
context.languages... I have an rpcserver cleanup patch I've been
working on which will include this change.
2. Remove deprecated gettext stuff in ipalib.request... this is a
small change, but I left it out of this patch so it's easier to
review
I'll have these next two patches later this week.
I've tested this and it works for me and seems pretty clean, a good
patch. Thank you Jason. However I do have one thing which I'd like to
see cleaned up, it's a few naming issues (see below).
In a moment I'm going to follow up with a patch that extends
tests/test_ipalib/test_text.py to utilize the test language you asked
for and is currently in install/po. That test is implemented and working
so look for the patch in a moment.
Naming Issues:
The thread local object can be assigned attributes directly and it's
attributes can be referenced directly. Using context.__dict__ seems odd
and unnecessary to store the language keys. I presume you're doing that
because you can't have a tuple as an attribute name on the context.
Directly accessing the __dict__ of an object feels like something we
should avoid if possible. Also we're stuffing unrelated items in
context.__dict__, for example the Connection and language keys are being
stored together. Wouldn't be cleaner to keep the language keys in their
own "name space" and to use constructs like this:
context = threading.local()
context.connection = Connection()
context.language_keys = {}
context.language_keys[key] = translation
if key in context.language_keys
rather than
context.__dict__[key] = translation
if key in context.__dict__
This also means when you clear the context you don't have to iterate
over the members of context.__dict__ and special case the values as is
currently being done with:
for (name, value) in context.__dict__.items():
if isinstance(value, Connection):
value.disconnect()
Wouldn't this be cleaner as:
if context.connection:
context.connection.disconnect()
Keeping the language keys separately would also allow us to clear the
language keys independently of anything else in the context without
having to worry about what else we might clobber in the context.
--
John Dennis
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] jderose 052 Finish deferred translation mechanism
This patch finishes the the LazyText functionality in the ipalib.text
module. This patch includes extensive docstrings in text.py that should
hopefully explain everything pretty well. There's also now pretty darn
complete test coverage. Still to do:
1. Have Backend.session extract the locale and set
context.languages... I have an rpcserver cleanup patch I've been
working on which will include this change.
2. Remove deprecated gettext stuff in ipalib.request... this is a
small change, but I left it out of this patch so it's easier to
review
I'll have these next two patches later this week.
>From 1b86cff2e402393c0ea8fdb472bb9cc665d2cda7 Mon Sep 17 00:00:00 2001
From: Jason Gerard DeRose
Date: Mon, 8 Mar 2010 20:42:26 -0700
Subject: [PATCH] Finish deferred translation mechanism
---
ipalib/__init__.py |2 +-
ipalib/parameters.py |6 +-
ipalib/request.py |5 +-
ipalib/text.py | 447 ++--
tests/test_ipalib/test_text.py | 135 +++-
tests/util.py |6 +-
6 files changed, 560 insertions(+), 41 deletions(-)
diff --git a/ipalib/__init__.py b/ipalib/__init__.py
index 51b63c9..6545bf7 100644
--- a/ipalib/__init__.py
+++ b/ipalib/__init__.py
@@ -881,7 +881,7 @@ from crud import Create, Retrieve, Update, Delete, Search
from parameters import DefaultFrom, Bool, Flag, Int, Float, Bytes, Str, Password,List
from parameters import BytesEnum, StrEnum, AccessTime, File
from errors import SkipPluginModule
-from text import _, gettext, ngettext
+from text import _, ngettext, GettextFactory, NGettextFactory
# We can't import the python uuid since it includes ctypes which makes
# httpd throw up when run in in mod_python due to SELinux issues
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index a598690..606a574 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -73,14 +73,14 @@ web-UI. The *label* should start with an initial capital. For example:
... label=_('Last name'),
... )
>>> sn.label
-Gettext('Last name')
+Gettext('Last name', domain='ipa', localedir=None)
The *doc* is a longer description of the parameter. It's used on the CLI when
displaying the help information for a command, and as extra instruction for a
form input on the web-UI. By default the *doc* is the same as the *label*:
>>> sn.doc
-Gettext('Last name')
+Gettext('Last name', domain='ipa', localedir=None)
But you can override this with the *doc* kwarg. Like the *label*, the *doc*
should also start with an initial capital and should not end with any
@@ -92,7 +92,7 @@ punctuation. For example:
... doc=_("The user's last name"),
... )
>>> sn.doc
-Gettext("The user's last name")
+Gettext("The user's last name", domain='ipa', localedir=None)
Demonstration aside, you should always provide at least the *label* so the
various UIs are translatable. Only provide the *doc* if the parameter needs
diff --git a/ipalib/request.py b/ipalib/request.py
index f21ac03..86b6433 100644
--- a/ipalib/request.py
+++ b/ipalib/request.py
@@ -52,10 +52,11 @@ def destroy_context():
"""
Delete all attributes on thread-local `request.context`.
"""
-# need to use .items(), 'cos value.disconnect modifies the dict
-for (name, value) in context.__dict__.items():
+# need to use .values(), 'cos value.disconnect modifies the dict
+for value in context.__dict__.values():
if isinstance(value, Connection):
value.disconnect()
+context.__dict__.clear()
def ugettext(message):
diff --git a/ipalib/text.py b/ipalib/text.py
index cabca43..96770ad 100644
--- a/ipalib/text.py
+++ b/ipalib/text.py
@@ -18,73 +18,480 @@
# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
"""
-Thread-local lazy gettext service.
+Defers gettext translation till request time.
-TODO: This aren't hooked up into gettext yet, they currently just provide
-placeholders for the rest of the code.
+IPA presents some tricky gettext challenges. On the one hand, most translatable
+message are defined as class attributes on the plugins, which means these get
+evaluated at module-load time. But on the other hand, each request to the
+server can be in a different locale, so the actual translation must not occur
+till request time.
+
+The `text` module provides a mechanism for for deferred gettext translation. It
+was designed to:
+
+1. Allow translatable strings to be marked with the usual ``_()`` and
+ ``ngettext()`` functions so that standard tools like xgettext can still
+ be used
+
+2. Allow programmers to mark strings in a natural way without burdening them
+ with details of the deferred translation mechanism
+
+A typical plugin will use the deferred translation like this:
+
+>>> from ipalib import Command, _, ngettext
+>>> class my_plugin(Command):
+... my_string = _('Hello, %(name)s.')
+...
