On Fri, Aug 28, 2009 at 9:41 PM, Johannes Dollinger
<johannes.dollin...@einfallsreich.net> wrote:
>
> The proposal and motivation are essentially the same as in the last
> thread [1] and the ticket description [2]. I put it on the 1.2 feature
> list.
> I tried to split my patch into smaller, more readable commits here:
> http://github.com/emulbreh/django/tree/master
> I haven't converted defaulttags, loadertags and django.templatetags.*
> yet to show that it's (mostly) backwards compatible (see below). It
> would be useful to decide on some questions in the "API Design .."
> section below first.
>
> I'll summarize the core ideas again as well as problems and pending
> design decisions.
>
> Concepts: Expressions and TokenStream
> =================================
> 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.
> This makes
> whitespace handling cumbersome and some syntactical constructs really
> hard to parse (e.g., the {% url ... %} parser is broken by design).
Saying {% url %} is "broken by design" doesn't accurately represent
the problem - it was a victim of inadequate tag building tools, and
now can't be fixed due to backwards-compatibility guarantees.
> Variable vs FilterExpression
> ======================
> I could only find documentation for Variable. If the internally used
> Parser.compile_filter() is indeed undocumented there is no official
> way to use FilterExpression in custom tags. If that means that
> FilterExpression.resolve() behaviour doesn't have to be backwards
> compatible, that would help a lot ..
As best as I can make out, you are correct. FilterExpression appears
to be an internal, so isn't covered by our backwards compatibility
guarantees. However, Variable is documented API.
> 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.
> 2.) Currently `Variable("a._hidden")` works - but
> `FilterExpression("a._hidden", p)` raises a TemplateSyntaxError. This
> would be unified: lookup parts may not start with an underscore. If
> it's not acceptable to break bc here leading underscores might simply
> be allowed.
I'm inclined to say this is a bug with FilterExpression. Underscored
variables shouldn't ever be exposed.
> 3.) Numeric literals ending with "." are currently treated as
> Variables. The following test (ifequal-numeric07) succeds because
> `ignore_failures=True` in IfEqualNode suppresses the inevitable
> VariableDoesNotExist exception and then compares None to 2: ('{%
> ifequal x 2. %}yes{% endifequal %}', {'x': 2}, ''). My current
> implementation raises a TemplateSyntaxError as soon as it encounters a
> numeric literal with a trailing dot. This could easily be made
> backwards compatible if it's considered worth it.
Sorry, but this one can't change. "2." is a tested format for handling
floats; the fact that this is inconvenient to your parser is
irrelevant.
> Tags
> =====
> * {% url ... %}, {% ssi ... %} currently accept unquoted literal
> strings. This will continue to work but the preferred syntax will use
> properly quoted strings. This may one day allow for viewnames and ssi
> paths that come from expressions.
At this point, this sort of fix can't happen until v2.0. It's would be
a major change at this point. It's nice to know that the template
parsing tools will handle it when the time comes, but I would expect
that to be true of _any_ rework of the parser.
> * {% if not %} would fail with my current implementation. This is
> fixable. But I'm not sure it's worth it: not/and/or should be keywords
> in the context of boolean expressions.
I'm unclear what you're proposing here. {% if not %} is a well
documented API, so it can't be deprecated.
****
There is a lot more work that requires a review here - not the least
of which is a large patch. I also know you've been working on this for
a long time - judging by the mailing list, at least a year, possibly
longer.
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.
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.
I'm also intrigued to know how this proposal intersects with Mike
Malone's patch to introduce template caching (#6262).
Please don't let this discourage you - I am keen to see improvements
in the template system. However, any widespread changes are going to
get _very_ thorough investigation, and before I invest the time
tearing down the code, I want to make sure the fundamentals
underpinning the patch are well understood and well founded.
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
-~----------~----~----~----~------~----~------~--~---
