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.
>>> 1.) Currently `Variable("unresolvable")` and
>>> `FilterExpression("unresolvable", p)` resolve differently: Variable
>>> raises VariableDoesNotExist and FilterExpression returns
>>> TEMPLATE_STRING_IF_INVALID. Except when `ignore_failures=True` is
>>> given. But `FilterExpression("foo|cut:unresolvable", p)` will again
>>> raise VariableDoesNotExist - regardless of `ignore_failures=True`.
>>>
>>> My Expression implementation unifies these behaviours: If any part of
>>> an expression is unresolvable a LookupError will be raised.
>>> `ignore_failures` will be deprecated but there's a resolve_safe()
>>> method on the Expression base class that reads:
>>>
>>> def resolve_safe(self, context, default=None):
>>> try:
>>> return self.resolve(context)
>>> except LookupError:
>>> return default
>>>
>>> This would be a small backwards incompatible change. I have one
>>> failing test (exception02) because the ExtendsNode implementation
>>> depends on the current FilterExpression behaviour.
>>
>> It's either backwards compatible or it isn't. Backwards compatibility
>> is a requirement, not an optional extra.
>
> If FilterExpression is not part of the public API this is a non-issue.
> The problem was that unresolvable variables were treated differently
> from unresolvable filter arguments.
While FilterExpression isn't part of the public API, the effects of
using FilterExpression _are_ part of public API. If I have code that
currently fails silently because of a badly named variable, *that
behaviour cannot change*. The fact that FilterExpression raises (or
doesn't raise) an exception is irrelevant. How that expression is
surfaced to the end-user is _very_ relevant.
>> I don't want to sound dismissive of the work you're done here, but
>> your comments in this post haven't inspired me to go looking deeper.
>> You've been a little cavalier with the term "backwards incompatible"
>> on a couple of occasions, which is something that simply isn't
>> acceptable in something as prominent and well documented as Django's
>> template language.
>
> Criticism is welcome. If FilterExpression is not public API ,
> everything should be backwards compatible now. That was the goal from
> the beginning.
> The {% if not %} and Variable("2.") fixes are ugly but compact.
> All tests pass (with two minor corrections: {% foo | bar %} is now
> valid and FilterExpression internals have changed).
The changes to internals don't especially concern me. However, I'm
unconvinced that "foo | bar" is a desirable change. I can see how it
would make your life easier, but unfortunately, that isn't the
criteria here :-)
>> There are bugs in the template system. Many of these are caused by
>> inconsistent handling of parsing, especially of variables. If you can
>> point at something that you think is a bug (such as the handlng of
>> foo._attrib), then it's no longer covered by backwards compatibility.
>> However, you don't get that accommodation simply because something
>> doesn't fit into your nice new parsing scheme.
>>
>> Now, some of this might just be a misunderstanding on my part -
>> especially with regards to what is partially implemented, what you
>> intend to fix, and what you think can't (or shouldn't) be fixed.
>> You've spent a lot of time describing your new parsing API, but not a
>> lot of time describing why the new API is required. To that end, I'm
>> not entirely sure how much of this patch is intended to cover the "fix
>> the inconsistencies in the template system" problem, and how much is
>> intended to fix the "make template tags easier to write" problem. If
>> you're proposing a major rework of internals with the goal of fixing
>> bugs, explaining exactly what is broken and how it is broken is an
>> important part of your proposal. If the goal is to improve the
>> usability of the public API, explaining the use cases you're trying to
>> make easier is important.
>
> 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].
[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?
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?
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.
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.
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-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
-~----------~----~----~----~------~----~------~--~---
