translation needs context but it doesn't always exist

On Thursday 24 February 2011, you wrote:
> Public bug reported:
>
> OpenERP v6:
>
> class GettextAlias in tools/translate.py requires a valid context
> available where _() was called. In some cases the context variable does
> not exsits or is not properly initialized. The attached patch provides a
> workarround by searching current user's language in order to find to
> which language it should translate.
>

I have 2 objections to that:
1. Speed. We want to avoid adding overhead to functions. The gettext one must
remain as light as possible (it is sometimes over-used, more than
appropriate).

2. Explicit/Implicit option to translate. In some cases, we explicitly don't
want to have a translation. So far, we have been doing that by not specifying
a 'lang' in the context. 1st example is the loading of a database and
'internal' operations. 2nd example is in server/bin/osv/orm.py:4052 where we
*must* avoid translation.

Please think twice before breaking this logic.

xrg, I don't agree.

1: It will not add any overhead because 99% of the cases context will already exist. Even more, 99% of the translations happen only when raising an exception so that means executing the code only once or twice in the RPC call.

2: If you do not want to translate, why do you mark the text to be translated? Even if you want that marked for some really strange reason, why not adding context['lang'] = 'en_US' ?

Not adding this piece of code makes the referential integrity error, which is very usual, appear always in English. I don't think that's acceptable having already a simple patch for it.

So, why don't we simply fix the 1% remaining where context is not passed right?
For the second one, 'en_US' does not mean "no translation". On the contrary, it still tries to locate en_US strings.
We'd better not break the framework (there is some reason gettext bails out when no language is in context, we have designed it to be so).

A Dissabte, 26 de febrer de 2011 20:51:42, xrg va escriure:
> So, why don't we simply fix the 1% remaining where context is not passed
> right? For the second one, 'en_US' does not mean "no translation". On the
> contrary, it still tries to locate en_US strings. We'd better not break
> the framework (there is some reason gettext bails out when no language is
> in context, we have designed it to be so).

Because workflow is still not ready to pass context. I provided a patch for v5
and was not accepted because it would break API.

It was not integrated into v6 either.

--
Albert Cervera i Areny
http://www.NaN-tic.com
OpenERP Partners
Tel: +34 93 553 18 03
skype: nan-oficina

http://twitter.com/albertnan
http://www.nan-tic.com/blog

Hello folks a few remarks. I tend to agree with Albert but I'm not 100% sure. Here is why:
- I agree 99% of the time we have the context so no perf overhead 99% of the time
- xrg: "why don't we simply fix the 1% remaining where context is not passed right?" Because this would probably imply changing the method API sometimes and this is not allowed (or should not be allowed) during the life of ""stable"" releases. Or may be I'm wrong and this is just about adding the context argument in method calls, but I doubt this is that benign.
So yes in the long-run let's fix it; but in the short run, Albert solution might be a good workaround.

I would add:
- xrg: when you don't want to translate: wouldn't that be possible to explicitly pass "en_US" in the context rather than nothing? Then no perf impact with uid.
- what do you think about that: Actually it's likely a user doesn't spent his time switching languages. So why not store a key 'uid':lang in some osv_memory object and if present we use it instead of hitting the database with the uid. If the user change it's language during his session, well, in 1% of cases he could have a wrong language, that's already much better than today. We could even drop the osv_memory key when this happen.

I know all this looks ugly. But IMHO this is "real politic", now 6.0 is supposed to be stable and changing methods API is no solution, we should do whatever is acceptable (and flagged properly as being a workaround) during those 6 months to fix usability. And yes, may be next time we should think twice about API stability in first place before freezing a release.

On Sunday 27 February 2011, you wrote:
>..
Again, you write a lot and don't read enough.. So, I will repeat the same
things for a THIRD time.

> ...Because this would probably
> imply changing the method API sometimes and this is not allowed (or should
> not be allowed) during the life of ""stable"" releases. .... Albert solution
might be a good workaround.

Albert's solution IS a change of API, and IMHO a significant one. Because, it
would introduce translation loops ( gettext is all over the place) where they
have never worked before.

> I would add:
> - xrg: when you don't want to translate: wouldn't that be possible to
> explicitly pass "en_US" in the context rather than nothing? Then no perf
> impact with uid.
Can you show me that part of the code? Because you seem to insist that en_US
would result in no translation.. I dare you.

> I know all this looks ugly. But IMHO this is "real politic", now 6.0 is
> supposed to be stable and changing methods API is no solution,
I'm talking about the same thing here. Changing the way the frameworks behaves
for a given set of input IS a change of API. Yes, sometimes the previous
behavior was not the best one (kept you seeing English strings that no Spanish
person understands, for example), but the rule is to improve things only "to
behave as expected". Getting translated strings from an empty context is not
what I would ever expect from the framework. (already gave you 2 examples
where this is the designed behavior).

A Diumenge, 27 de febrer de 2011 11:48:28, xrg va escriure:
> I'm talking about the same thing here. Changing the way the frameworks
> behaves for a given set of input IS a change of API. Yes, sometimes the
> previous behavior was not the best one (kept you seeing English strings
> that no Spanish person understands, for example), but the rule is to
> improve things only "to behave as expected". Getting translated strings
> from an empty context is not what I would ever expect from the framework.
> (already gave you 2 examples where this is the designed behavior).

Well, its your decision after all. But just for the record of other users
we've got this patch applied in v5 in several production systems for months
now with no issues so far. We're going to use it in production systems for v6
too.

--
Albert Cervera i Areny
http://www.NaN-tic.com
OpenERP Partners
Tel: +34 93 553 18 03
skype: nan-oficina

http://twitter.com/albertnan
http://www.nan-tic.com/blog

One question.

I'm agreed with Albert, BTW why xrg don't take a branch and try with test.openobject.com a look for some broken in the middle? on V5.0 Albert is saying that it is not broking anything, don't yo think you can try it as we (community) are doing?, some times is better try this kind of changes before post for a milestones (i'm agreed with xrg in this point), BUT Albert (the creator ) try it already, you (quality team) without try yet, only for a conceptual rule are saying NO,

IMHO quality team needs to try things before say "No because it _can_ brake something" if you try before answer it, and post your results, I think (Albert correct me please if i'm wrong) Albert can extend the patch and you apply it in a really generic way.

Thanks.

Thank you for the new patch Albert.

Here is my view.
- I personally think, the patch paves a way to translate the warning/exception if anyhow the context is not passed to any method(note that this can be any method which is not an ORM method,could be your own written by you and it raises a warning),correct me if I am wrong.
- The only thing I fear is, the arguments. If the programmer is using arguments other than cr,cursor,uid,user; this could lead to a waste of time. Might be,Xrg is thinking this overhead,but this happens only in less than 1% of cases.

As far as the execution of this code is concerned, this is again gonna happen in very rare number of cases.

In my view, as I already know why this was added on bug 432504 , this is like fixing a leak of a pipe(as things work 99% accurate due to the presence of context).

Thanks.