On Tue, Oct 26, 2010 at 8:00 AM, Benjamin Wohlwend <piquad...@gmail.com> wrote:
> Hi,
>
> Russell, thanks for having a look at this. Much appreciated!
>
> On Mon, Oct 25, 2010 at 4:04 PM, Russell Keith-Magee
> <russ...@keith-magee.com> wrote:
>> Responding so that
>>
>> "localize off" is a much better approach to localize
>>
>> 1) I think there is still a need for a template filter. There's an
>> analog here with autoescape; there's an autoescape template tag for a
>> large block of content, but there's also an escape filter to escape a
>> single variable.
>
> The revised patch has a template filter. Not sure about using the same
> name as for the template tag, though.
>
>>
>> 2) activate() isn't a cheap operation; I know I put {% localize "de"
>> %} as a use case in the ticket, but on reflection, I'm not sure it's
>> worth the overhead. That said, I'm willing to be convinced otherwise
>> if there's a lot of demand for this feature.
>
> I agree that switching locale in the template rendering stage seems to
> be an obscure use case. I removed it for the time being.
>
>>
>> 3) The '_localize' magic variable in the context. It looks to me like
>> a better solution would be to pass down a 'localize' argument to
>> render. This is a little more intrusive to the codebase, but it
>> doesn't have the same code smell to me.
>
> I tried to implement this, but this puts me firmly into code paths I'm
> not comfortable messing with, especially with regard to backwards
> compatibility. I confined the `localization` argument to
> `VariableNode`'s with an `isinstance` check, but still, any third
> party `Node`s inheriting from `VariableNode` will not like this.
>
>>
>> 4) 'force' isn't a good description of the argument to date_format()
>> et al. I'd be inclined to extend the 'localize' argument from point
>> (2), and use a ternary logic value; True/False, plus None == use the
>> value of USE_L10N.
>
> Done. I named the argument `localization`, though. `localize` crashes
> with the `localize` function in `django.utils.formats`.
I've just done a polish of your patch. Looking at the changes required
by render() started to make me a little nervous, since they required
changing the API for render, so I went looking for a better way -- and
found it with autoescape. You don't have to create a context variable
to store something in the context. Given that localization is a core
concept in Django, keeping localization state in the context makes
sense, so I've modified your patch to use that approach.
I've also separated the tags into their own tag library, keeping the
analogy with the i18n tag library.
I've uploaded the patch to the ticket; I want to get a couple of key
people to comment on it before I commit, but with any luck this should
land in the next day or so.
Thanks for your work on this!
Yours,
Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
