Am 29.09.2009 um 04:07 schrieb Russell Keith-Magee:
>
> On Tue, Sep 29, 2009 at 12:44 AM, Johannes Dollinger
> <johannes.dollin...@einfallsreich.net> wrote:
>>
>>>> Variable and FilterExpression will be deprecated as they require
>>>> the
>>>> caller to know the length of the expression in advance.
>>>
>>> I'm not sure I follow this assertion. I have no problem believing
>>> FilterExpression can be made cleaner, but Variable takes an
>>> expression
>>> like 'article.section', plus a context, and resolves it into a
>>> string.
>>> In itself, it seems like a well built and stable piece of API.
>>
>> To get this expression string in the first place you have to split
>> Token.contents somehow. smart_split() does that well for space
>> delimited expressions.
>> But if you try to parse something like {% url %} you have to split
>> its
>> results again: "foo.bar,baz|filter:arg,42" won't do it. The current
>> {%
>> url %} parser now resorts to `str.split(",")` and
>> {% url viewname list|join:"," %}
>> will break it. Of course that's a corner case and could be fixed with
>> something like `smart_split(delim=",")`. But it would be much more
>> elegant if you could just read an expression from a stream of tokens.
>> That's what I meant when I said "the {% url %} parser is broken by
>> design". I didn't mean to sound harsh - {% url %}'s parser is a
>> reasonable tradeoff between simplicity and correctness.
>
> Sure - that explains why FilterExpression is a problem, but I don't
> see why Variable is affected. Once you've parsed your expression into
> tokens, those tokens that are known to be variables need to convert
> 'foo.bar' into something derived from context.
The idea was that in most cases the template tag doesn't care if you
pass in a Variable or a FilterExpression (#5756). So ideally there was
only one method that returns something with a .resolve(Context)
method. I know that Variable has to remain functional - and it is.
It's a 20 lines wrapper around the TokenStream API:
http://github.com/emulbreh/django/blob/master/django/template/compat.py#L8
>> It's both: making the API friendlier and fixing bugs (by using the
>> higher level API).
>> I thought that motivation and scope were mostly clear and approved.
>> Let me refer you to the two previous threads for now:
>>
>> http://groups.google.com/group/django-developers/browse_thread/thread/fb246422fa4e23d8
>> http://groups.google.com/group/django-developers/browse_thread/thread/4f5b01719e497325
>
> Herein lies the problem. The message you appear to have taken from
> those threads doesn't match with what I see. I see:
>
> * Agreement that work is needed on templates
> * A long debate about a blog post comparing Jinga and Django's
> templates.
> * Some very specific comments from Malcolm addressing your approach
> to backwards compatibility [1]
> * Agreement that this is a very large body of work
> * Agreement that the API for custom tags is too verbose.
> * Comments that the process of fixing bugs should be independent of
> improving API [2]
>
> [1] http://groups.google.com/group/django-developers/msg/97d32ac923b1f700
> [2] http://groups.google.com/group/django-developers/msg/100e70150f37c172
>
> On these points, you won't get any argument from me.
>
> What I don't see is any specific endorsement of your approach, beyond
> a general "looks like you're going the right way" comment from Jacob
> [3]. However, that comment could be leveled at _any_ attempt to
> rewrite the parser - after all, it's the parser that is the problem.
> Your approach specifically _doesn't_ address Malcolm's concerns [2].
My approach was: create a new API for TagParsers then use this API for
all tag libraries in django. All bug fixes just fell out of this. I
don't see the point in fixing those bugs with the old API first and
then throwing away the code. And I understand that backwards
compatibility is important, there exist no backwards compatibility
issues I know of (FilterExpression behaviour was the only major
problem).
> [3] http://groups.google.com/group/django-developers/msg/6e03c7c1a5b3d946
>
> Your code also seems to be a moving target. My original impression was
> that you were calling for a major review of a potentially trunk-ready
> patch. Deeper reading revealed that there were potentially lots of
> backwards incompatibilities. Reading subsequent messages reveals that
> maybe those backwards incompatibilities don't exist any more. At this
> point, I'm straight up confused as to the state of this patch - is it
> trunk ready, or a work in progress? Are you looking for a design
> review or a final review? If it's a design review, what aspects of the
> design do you want feedback on?
It's definitly not trunk-ready. There have been no comments on the
TokenStream API yet and some minor design decisions remain. Again, in
it's current state there exist no backwards incompatibilites I know
of. I'm asking for review of the concept, the API and finally the code.
> Reviewing this code will be a big job. If you want this to move
> forward, you need to make it easy and obvious for me (and/or the other
> core developers) to make the decision to commit to reviewing your
> code. This is one of those situations that has almost nothing to do
> with the code, and everything to do with effective communication. What
> have you done? Why have you done it that way? What alternatives did
> you consider? What tradeoffs exist? What potential problems exists?
I added TokenStream (I'm not happy with the name) a class that
tokenizes Token.contents and provides methods to conveniently read
expressions, literals and keywords from it. Expressions are objects
with a .resolve(Context) method. There currently are Literal, Lookup,
and FilterExpression classes in django.template.expressions. These
changes do not affect any of the existing functionallity - they are
just a new API . There's a decorator for tag parser functions
(@uses_token_stream) that will automatically replace the Token
argument with a TokenStream.
While this could easily live outside of django, problems with builtin
tags would remain. So I refactored Variable, and
Compiler.parse_filter() as well as all tag libraries to use this new
API.
Alternatives would be to merge FilterExpression and Variable and fix
bugs using the existing API (as Malcolm suggested). Or additions to
smart_split() to make it easier to tokenize Token.contents.
But that would hardly make custom tags easier to write (or to get
correct). Therefore I wrote a parser for Token.contents that doesn't
require you to deal with raw strings.
One major problem may be performance. The new code is slower (no real
benchmarks, just comparing the time spent in the tests) and probably
uses more memory.
Apart from that, the only critical decision is what should be
considered a token:
http://github.com/emulbreh/django/blob/master/django/template/parser.py#L351
Optionally tag parsers could specify their own token regexps, which
would make tags like {% smart_if %} easier to parse; but that could
also be achieved by subclassing TokenStream.
> You need to convince me that it's worth spending the time to review
> your code. You don't have an extensive collection of smaller patches
> or a history of engaging in design discussions on django-dev, so all I
> can use to establish your code design credentials is your ability to
> communicate your ideas about this ticket on the mailing list. If I
> have to spend a lot of time working out what is going on before I even
> get to the code, that doesn't bode well.
If you want to get an impression of the API have a look at parsers in
defaulttags.py
(http://github.com/emulbreh/django/blob/master/django/template/defaulttags.py
).
TokenStream is about 200 LOC
(http://github.com/emulbreh/django/blob/master/django/template/parser.py#L382
) and expressions.py has 138 LOC
(http://github.com/emulbreh/django/blob/master/django/template/expressions.py
). I know that's still a lot to review but I hope it's short enough
for a quick glance to get an impression of the design.
> Again - please don't be discouraged. I want to see the template system
> improved. If you're sufficiently enthused to pursue the topic, I
> encourage you to do so. But you need to effectively communicate what
> you have done along the way.
I'm not passionate about this patch. But I'd like to know if it has
some kind of future or should be thrown away.
__
Johannes
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@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
-~----------~----~----~----~------~----~------~--~---
