[Freeipa-devel] [PATCH] jderose 052 Finish deferred translation mechanism

[Jason Gerard DeRose]

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.