On Wednesday 09 December 2009 12:43:04 Russell Keith-Magee wrote:
> I've done another review, and it's looking pretty good. I have
> three relatively minor comments:
>
> * There is what appears to be vestigial documentation around line
> 346 of ref/templates/builtins.txt; it says evaluation order is
> left-to-right, rather than operator precedence as described later
> on.
Thanks, changed.
> * Most of the lambdas you have defined in smartif.py are already
> available in the builtin operator module - e.g., operator.or_,
> operator.and_, operator.gt, etc.
Oh yeah, forgot about those!
> * The handling of nodelist_false in IfNode isn't wrong (as in, it
> doesn't generate wrong results), but it was contrary to my
> expectations. I would have expected a simple "else:
> self.nodelist_false.render()". Is there a particular reason for
> this? I made the change in source and it didn't break any tests.
>
> The only two reasons I could come up with to explain this approach
> were that it would be marginally faster to return '' rather than
> render an empty nodelist, and/or an old-school mandate that every
> function should have a return statement at the end. I'm not
> necessarily saying this should be changed; but at the very least,
> it warrants an explanatory comment as to why the obvious approach
> isn't being used.
I don't know the reason either, it's part of Chris' original patch,
and I've simplified it now.
> This is a hard one. Keeping with our tradition of disagreeing with
> each other, I'm actually inclined to go the other way :-)
>
> I have two reasons. Firstly, I'm of the opinion that templates
> shouldn't ever throw exceptions during rendering - they should fail
> silently. Differentiating between False and Exception may be
> difficult or inconvenient for the developer, but it's even more
> inconvenient for the end user to see a 500 IMHO.
You're right, and I was misreading the current code in thinking that
it currently raises exceptions during rendering. While Variable and
others raise exceptions, they are caught higher up. (There are just
occasional places where builtins propagate exceptions, like {% url
%}).
> Secondly, while I can see the analog with silent_variable_failure,
> it isn't quite the same circumstances. Most of the interesting
> functions that will be invoked in templates will be internal to
> Django or user-code, so we have almost full control over the
> silent_variable_failure attribute. We can set
> silent_variable_failure on DoesNotExist because it is internal to
> Django, and it will behave nicely when errors occur in templates.
>
> However, to my reckoning, this isn't true for operators. Many, if
> not most of the interesting use cases for operators will be
> comparisons with literals, sets, lists, or other Python builtins -
> which throw exceptions which we can't control. We may well honor a
> silent_variable_failure attribute, but if it's near impossible to
> actually set that attribute in practice, there isn't much point in
> honoring it in the first place.
Very good point. You convinced me easily, contrary to tradition this
time :-)
Thanks for the review again.
Luke
--
If you can't answer a man's arguments, all is not lost; you can
still call him vile names. (Elbert Hubbard)
Luke Plant || http://lukeplant.me.uk/
--
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.
