[Freeipa-devel] [PATCH] Send Accept-Language header over XML-RPC and translate on server.

[Pavel Zůna]

On 2011-02-04 16:23, Rob Crittenden wrote:
> Pavel Zuna wrote:
>> This patch makes the ipa client send the Accept-Language header, so that
>> the server can translate things like exceptions, that cannot be
>> translated on the client.
>>
>> It also fixes the language recognition for the webUI. The values in
>> Accept-Language header are a bit different than what is accepted by the
>> LANG variable as a valid locale - some additional parsing was needed.
>> For example:
>> >>> Accept-Language: es-es;q=1
>> needs to translate to
>> >>> es_ES
>> otherwise it won't be recognized by gettext
>>
>> Fix #904
>> Fix #917
>>
>> Pavel
>
> nack.
>
> ast is imported but not used

Leftover. Removed in the attached updated version.

> Why are you calling locale.setlocale() instead of locale.getlocale()?

Because that's how it should be done. setlocale() with an empty string 
as second argument gets the current environment settings. getlocale() 
without a previous call to setlocale returns (None, None).

> If extra_headers is passed in as a string this will drop it:

That's never going to happen. I checked the underlying implementation in 
xmlrpclib and it can either be a list or dict. In this case, 
LanguageAwareTransport is calling Transport.get_host_info() which always 
returns extra_headers as a list or None if empty.

The original implementation (before this patch) always dropped the whole 
thing and used a new list instead.

> + if not isinstance(extra_headers, list):
> + extra_headers = []
>
> Multiple Authorization is actually legal though it may be a good idea to
> remove any others found, so I'll let this part go. I don't know that it
> is really needed though.

Because the underlying Transport class can fill Authorization with 
'Basic <auth>' and the original implementation was dropping it as well.

> Some formatting is changed to make it less readable IMHO:
>
> - else:
> - scheme = "http"
> + else: scheme = "http"

That's unintentional, sorry.

> The code to break HTTP_ACCEPT_LANGUAGE into language and region is
> broken. Passing in en-gb returns en_EN. (I think you want [1] not [0]).

Nice catch. I was probably thinking that since I'm using rsplit(), the 
indexes will be the other way around. :) Fixed in attached version.

> Ideally we would loop through all acceptable languages until we find one
> that we actually provide.
>
> So if we are passed in da, en-gb;q=0.8, en;q=0.7 we would first look for
> Danish but fall back to British English or any other English (preferring
> British English).

That's a good idea! However I would keep it simple for now and do this 
in a separate patch.

> rob

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pzuna-71-2-acceptlang.patch
Type: application/mbox
Size: 3955 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110204/3bd7001e/attachment.mbox>