Re: smart if tag

2009-12-09 Thread Luke Plant 
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.

Re: smart if tag

2009-12-09 Thread Russell Keith-Magee 
On Wed, Dec 9, 2009 at 9:17 AM, Luke Plant  wrote:
> Hi all,
>
> I've now addressed everything in Russell's last e-mail, I think, so I
> think I'm pretty much good to go, apart from:
>
> 1) my last change rewrote a lot of IfParser, which was the heart of
> the patch. That means it probably needs looking at again! On the other
> hand, the new implementation is based on a much more general and more
> tested parser methodology, so it should hopefully be pretty solid.

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.

 * 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.

 * 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.

Other than the issue you raise below, I'm happy. In particular, I'm
much happier with the parsing code now - I found it much less
confusing that the previous iteration, and it has the added benefit of
being correct :-)

> 2) what should be done about errors with operators?  For the most
> part, it isn't much of an issue, as 'object' defines many of the magic
> methods involved.  For 'in', however, you are likely to get TypeErrors
> if the object doesn't support it. Currently missing variables are
> handled gracefully, but {% if 'x' in 1 %} will get you a TypeError
> that is not caught.  And there is still the possibility that other
> operators will cause errors.
>
> Our current policies don't quite handle this situation, AFAICS.  For
> the case of 'in', catching any Exception and returning False could be
> defended - to use the above example, 1 is not an object that contains
> 'x', therefore it should return False.  This would protect template
> authors against various exceptions.
>
> Or we could go with the policy for method calls, which is to check the
> exception for 'silent_variable_failure' attribute. This has the
> advantage that inappropriate use of 'in' by template authors, which is
> almost always going to be a mistake, will throw a loud error. The
> error message in this case isn't particularly obvious though:
> "TypeError: argument of type 'int' is not iterable"
>
> I'm leaning towards the latter. If a view changes so that, in the
> variables passed to the template, a container with a __contains__()
> method is replaced by one without, or by one with a buggy
> __contains__() method that throws exceptions, it's more useful and
> arguably more correct to get an exception than for the 'if' tag
> expression to return False.

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.

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.

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

Re: smart if tag

2009-12-06 Thread Alex Gaynor 
The undcoumented scanner class in the regex module may interest you:
http://code.activestate.com/recipes/457664/

Alex

On Sun, Dec 6, 2009 at 3:32 AM, SmileyChris  wrote:
> Because that link intrigued me, I challenged myself to write my own
> generic lexer & parser based on what I had read:
> http://gist.github.com/250128
>
>
>
> On Dec 6, 2:07 pm, Luke Plant  wrote:
>> On Saturday 05 December 2009 20:09:21 Luke Plant wrote:
>>
>> > I'm not likely to able to look at this before Tuesday.  If anyone
>> > wants to look at it, I think the right approach is something like
>> >  the following:
>> >http://effbot.org/zone/simple-top-down-parsing.htm
>> > (without the globals, obviously, they can be converted to instance
>> > variables/methods).
>>
>> Cancel that - I unexpectedly had free time this evening, and I
>> implemented this.  It's a nice replacement I think (credit to Vaughan
>> Pratt and Fredrik Lundh for the basic approach and Python
>> implementation respectively).  The new implementation is pretty much
>> the same length as the old one, and hopefully easier to read, with a
>> much smaller core parser.  Precedence is specified directly, rather
>> than implicitly, so it's much easier to check that it's the same as
>> Python's.
>>
>> Latest patch attached to this e-mail.
>>
>> Regards,
>>
>> Luke
>>
>> --
>> "Idiocy: Never underestimate the power of stupid people in large
>> groups." (despair.com)
>>
>> Luke Plant ||http://lukeplant.me.uk/
>>
>>  smartif_8.diff
>> 27KViewDownload
>
> --
>
> 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.
>
>
>



-- 
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me

--

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.

Re: smart if tag

2009-12-06 Thread SmileyChris 
Because that link intrigued me, I challenged myself to write my own
generic lexer & parser based on what I had read:
http://gist.github.com/250128



On Dec 6, 2:07 pm, Luke Plant  wrote:
> On Saturday 05 December 2009 20:09:21 Luke Plant wrote:
>
> > I'm not likely to able to look at this before Tuesday.  If anyone
> > wants to look at it, I think the right approach is something like
> >  the following:
> >http://effbot.org/zone/simple-top-down-parsing.htm
> > (without the globals, obviously, they can be converted to instance
> > variables/methods).
>
> Cancel that - I unexpectedly had free time this evening, and I
> implemented this.  It's a nice replacement I think (credit to Vaughan
> Pratt and Fredrik Lundh for the basic approach and Python
> implementation respectively).  The new implementation is pretty much
> the same length as the old one, and hopefully easier to read, with a
> much smaller core parser.  Precedence is specified directly, rather
> than implicitly, so it's much easier to check that it's the same as
> Python's.
>
> Latest patch attached to this e-mail.
>
> Regards,
>
> Luke
>
> --
> "Idiocy: Never underestimate the power of stupid people in large
> groups." (despair.com)
>
> Luke Plant ||http://lukeplant.me.uk/
>
>  smartif_8.diff
> 27KViewDownload

--

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.

Re: smart if tag

2009-12-05 Thread Luke Plant 
On Saturday 05 December 2009 20:09:21 Luke Plant wrote:

> I'm not likely to able to look at this before Tuesday.  If anyone
> wants to look at it, I think the right approach is something like
>  the following:
> http://effbot.org/zone/simple-top-down-parsing.htm
> (without the globals, obviously, they can be converted to instance
> variables/methods).

Cancel that - I unexpectedly had free time this evening, and I 
implemented this.  It's a nice replacement I think (credit to Vaughan 
Pratt and Fredrik Lundh for the basic approach and Python 
implementation respectively).  The new implementation is pretty much 
the same length as the old one, and hopefully easier to read, with a 
much smaller core parser.  Precedence is specified directly, rather 
than implicitly, so it's much easier to check that it's the same as 
Python's.

Latest patch attached to this e-mail.

Regards,

Luke

-- 
"Idiocy: Never underestimate the power of stupid people in large 
groups." (despair.com)

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.


diff -r 70e75e8cd224 django/template/defaulttags.py
--- a/django/template/defaulttags.py	Thu Dec 03 15:11:14 2009 +
+++ b/django/template/defaulttags.py	Sun Dec 06 01:04:04 2009 +
@@ -11,6 +11,7 @@
 from django.template import Node, NodeList, Template, Context, Variable
 from django.template import TemplateSyntaxError, VariableDoesNotExist, BLOCK_TAG_START, BLOCK_TAG_END, VARIABLE_TAG_START, VARIABLE_TAG_END, SINGLE_BRACE_START, SINGLE_BRACE_END, COMMENT_TAG_START, COMMENT_TAG_END
 from django.template import get_library, Library, InvalidTemplateLibrary
+from django.template.smartif import IfParser, Literal
 from django.conf import settings
 from django.utils.encoding import smart_str, smart_unicode
 from django.utils.itercompat import groupby
@@ -227,10 +228,9 @@
 return self.nodelist_false.render(context)
 
 class IfNode(Node):
-def __init__(self, bool_exprs, nodelist_true, nodelist_false, link_type):
-self.bool_exprs = bool_exprs
+def __init__(self, var, nodelist_true, nodelist_false=None):
 self.nodelist_true, self.nodelist_false = nodelist_true, nodelist_false
-self.link_type = link_type
+self.var = var
 
 def __repr__(self):
 return ""
@@ -250,28 +250,11 @@
 return nodes
 
 def render(self, context):
-if self.link_type == IfNode.LinkTypes.or_:
-for ifnot, bool_expr in self.bool_exprs:
-try:
-value = bool_expr.resolve(context, True)
-except VariableDoesNotExist:
-value = None
-if (value and not ifnot) or (ifnot and not value):
-return self.nodelist_true.render(context)
+if self.var.resolve(context):
+return self.nodelist_true.render(context)
+if self.nodelist_false:
 return self.nodelist_false.render(context)
-else:
-for ifnot, bool_expr in self.bool_exprs:
-try:
-value = bool_expr.resolve(context, True)
-except VariableDoesNotExist:
-value = None
-if not ((value and not ifnot) or (ifnot and not value)):
-return self.nodelist_false.render(context)
-return self.nodelist_true.render(context)
-
-class LinkTypes:
-and_ = 0,
-or_ = 1
+return ''
 
 class RegroupNode(Node):
 def __init__(self, target, expression, var_name):
@@ -761,6 +744,27 @@
 return do_ifequal(parser, token, True)
 ifnotequal = register.tag(ifnotequal)
 
+class TemplateLiteral(Literal):
+def __init__(self, value, text):
+self.value = value
+self.text = text # for better error messages
+
+def display(self):
+return self.text
+
+def resolve(self, context):
+return self.value.resolve(context, ignore_failures=True)
+
+class TemplateIfParser(IfParser):
+error_class = TemplateSyntaxError
+
+def __init__(self, parser, *args, **kwargs):
+self.template_parser = parser
+return super(TemplateIfParser, self).__init__(*args, **kwargs)
+
+def create_var(self, value):
+return TemplateLiteral(self.template_parser.compile_filter(value), value)
+
 #...@register.tag(name="if")
 def do_if(parser, token):
 """
@@ -805,47 +809,21 @@
 There are some athletes and absolutely no coaches.
 {% endif %}
 
-``if`` tags do not allow ``and`` and ``or`` clauses with the same tag,
-because the order of logic would be ambigous. For example, this is
-invalid::
+

Re: smart if tag

2009-12-05 Thread Luke Plant 
First, thanks for your review Russell, I appreciate that you've been 
doing tons of work on lots of things!

On Saturday 05 December 2009 15:39:03 Russell Keith-Magee wrote:

> Here's the review I promised. First the minor points:
> 
>  * Line 814 of templates/defaulttags.py has a wierd UTF-8 character
>  - an emdash, rather than an ASCII hyphen. I don't know what's
>  happening on your system, but this causes crashes for me
>  complaining about PEP-0263 problems.

I did add a 'coding' line to fix that, but perhaps after you pulled my 
latest code.  Anyway, it's not worth it so I removed it.

>  * The if-tag-not-02/05 tests that have been removed should be
> retained, but be testing for the TemplateSyntaxError (just like the
> if-tag-error tests do). If we're going to implement a new language
> contract and introduce some reserved words, we should test that
>  that those words are actually reserved.

There are actually other tests that check this, in the IfParser tests. 
I didn't see the usefulness of 5 different tests all checking that 
'not' is a keyword, but renumbering the following tests would be 
worse, hence the note about why they were missing. Also, it makes more 
sense to put the syntax error tests together.

>  * I'm not wild about the docs for the if tag referring how
> comparisons work "just like Python operators". We have historically
> made a big deal of templates not being Python, and while we are
> blurring the lines a little with this patch, I'd still like to
> maintain the pretense. The preceding notes about logical operators
>  is a good example of how it should be done IMHO - it gives
>  examples of how and/or work without needing to resort to saying
>  "just like Python".

Fixed now, hopefully, I've given examples for all the operators.

>  * The explanation of why {% if a > b > c %} doesn't work is a bit
> vague. At the very least, it should have an example of the correct
> implmentation - {% if a > b and b > c %}.

Fixed now.

> Now the major issues: There's only one that I found - the parsing
> strategy for complex logical expressions. "A or B and C" is parsed
>  as "(A or B) and C", not the pythonic way "A or (B and C)" (giving
>  operator precedence to AND over OR).
> 
> Personally, I found this very surprising. When I said in a previous
> email that I didn't think it was worth hobbling the if statement to
> prevent complex logical operations, I presumed it was going to be
> replaced with a full parser. Historically, it hasn't been an issue
> because we've only allowed one type of logical operator in an {% if
> %}. I think I'd rather see this tradition continued rather than
>  allow mixing in a way that will be confusing to anyone with prior
>  programming experience.

Good catch.  I think it would be nicer to fix than just disallow, 
although it could be a substantial change to the way the IfParser 
class works. I don't think it is too much work, however, because the 
hard bit is tokenizing, which has already been done.

I'm not likely to able to look at this before Tuesday.  If anyone 
wants to look at it, I think the right approach is something like the 
following: 
http://effbot.org/zone/simple-top-down-parsing.htm
(without the globals, obviously, they can be converted to instance 
variables/methods).

I don't think trying to hack this on to the current IfParser would be 
a good idea.  IfParser already has a slightly hacky way of handling 
the precedence of 'and' and 'or' relative to the other operators, and 
this is a good opportunity to replace that with something nicer.

>  Lastly, a controversial topic, but I think we need an answer on
>  this - whither {% ifequal %}? Should we be deprecating it? Given
>  the current level of usage of the ifequal/ifnotequal tags, this
>  seems excessive. Perhaps we should consider this for long-term
>  deprecation (i.e., flag it as a 2.0 deprecation)?

Yes, I see no value in deprecating this until 2.0, given how much it 
is used.

However, while I remember, there is this bug which we may need to 
revisit:

http://code.djangoproject.com/ticket/10975

Essentially, you can't use {% block %} inside an {% if %} tag, but you 
can in an {% ifequal %} tag.  Irrespective of which one is right, it 
seems unreasonable that {% if x == y %} behaves so differently from 
{% ifequal x y %} in this regard. (Actually, I didn't test that. 
Perhaps the behaviour of 'if' has changed with the smart if code).

Regards,

Luke


-- 
"Idiocy: Never underestimate the power of stupid people in large 
groups." (despair.com)

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.

Re: smart if tag

2009-12-05 Thread Russell Keith-Magee 
On Tue, Dec 1, 2009 at 9:55 AM, Russell Keith-Magee
 wrote:
> On Tue, Dec 1, 2009 at 1:08 AM, Luke Plant  wrote:
>> Patch
>> =
>>
>> Review would be welcome, especially as I'm ill at the moment. I'm only
>> coding because the boredom of doing nothing is killing me...
>
> I'll try and take a look over the next couple of days.

Here's the review I promised. First the minor points:

 * Line 814 of templates/defaulttags.py has a wierd UTF-8 character -
an emdash, rather than an ASCII hyphen. I don't know what's happening
on your system, but this causes crashes for me complaining about
PEP-0263 problems.

 * The if-tag-not-02/05 tests that have been removed should be
retained, but be testing for the TemplateSyntaxError (just like the
if-tag-error tests do). If we're going to implement a new language
contract and introduce some reserved words, we should test that that
those words are actually reserved.

 * I'm not wild about the docs for the if tag referring how
comparisons work "just like Python operators". We have historically
made a big deal of templates not being Python, and while we are
blurring the lines a little with this patch, I'd still like to
maintain the pretense. The preceding notes about logical operators is
a good example of how it should be done IMHO - it gives examples of
how and/or work without needing to resort to saying "just like
Python".

 * The explanation of why {% if a > b > c %} doesn't work is a bit
vague. At the very least, it should have an example of the correct
implmentation - {% if a > b and b > c %}.

 * The interaction between this patch and #6262 will be fun. (This is
more a mental note to myself that I should commit #6262)

Now the major issues: There's only one that I found - the parsing
strategy for complex logical expressions. "A or B and C" is parsed as
"(A or B) and C", not the pythonic way "A or (B and C)" (giving
operator precedence to AND over OR).

Personally, I found this very surprising. When I said in a previous
email that I didn't think it was worth hobbling the if statement to
prevent complex logical operations, I presumed it was going to be
replaced with a full parser. Historically, it hasn't been an issue
because we've only allowed one type of logical operator in an {% if
%}. I think I'd rather see this tradition continued rather than allow
mixing in a way that will be confusing to anyone with prior
programming experience.

Lastly, a controversial topic, but I think we need an answer on this -
whither {% ifequal %}? Should we be deprecating it? Given the current
level of usage of the ifequal/ifnotequal tags, this seems excessive.
Perhaps we should consider this for long-term deprecation (i.e., flag
it as a 2.0 deprecation)?

Russ %-)

--

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.

Re: smart if tag

2009-11-30 Thread Russell Keith-Magee 
On Tue, Dec 1, 2009 at 1:08 AM, Luke Plant  wrote:
> On Monday 30 November 2009 11:18:13 Russell Keith-Magee wrote:
>
>> > 1) Handling non-existent variables
>> > ==
> So, I'm *now* suggesting that we convert everything missing to None.

Sounds good to me.

> Behaviour of 'not' - revisited
> ==
>
> I propose a backwards incompatibility here, it looks like this:
>
>  The 'if' tag no longer accepts 'and', 'or' and 'not' as valid
>  variable names.  Previously that was allowed in some cases even
>  though they were normally treated as keywords.  Now, the keyword
>  status is always enforced, and code like {% if not %} or {% if and %}
>  will throw a TemplateSyntaxError.
>
> This would now be the only backwards incompatibility (discounting the
> possibility that people were relying on TemplateSyntaxError for things
> that are now legal).

Also sounds good.

> Patch
> =
>
> Review would be welcome, especially as I'm ill at the moment. I'm only
> coding because the boredom of doing nothing is killing me...

I'll try and take a look over the next couple of days.

Russ %-)

--

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.

Re: smart if tag

2009-11-30 Thread Luke Plant 
On Monday 30 November 2009 20:27:32 SmileyChris wrote:
> On Dec 1, 6:08 am, Luke Plant  wrote:
> > Given that the 'Null' stuff has now been removed, we could
> > move back to your way to reduce the code a bit, but I'm not sure
> > it is worth it.
> 
> I'd say reduce the code again.

I've removed Less, LessOrEqual and NotEqual.  I kept 'Not' as I think 
it's slightly cleaner than putting 'negate' into BaseCalc. It also 
removes the asymmetry of using 'Or' at one point in the code, which I 
found slightly puzzling. 

> And I think you missed adding some files in your repo?

Doh!  Added now, along with some docs.

Regards,

Luke

-- 
"Humiliation: The harder you try, the dumber you look." 
(despair.com)

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.

Re: smart if tag

2009-11-30 Thread SmileyChris 
On Dec 1, 6:08 am, Luke Plant  wrote:

> Given that the 'Null' stuff has now been removed, we could
> move back to your way to reduce the code a bit, but I'm not sure it is
> worth it.

I'd say reduce the code again.

And I think you missed adding some files in your repo?

--

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.

Re: smart if tag

2009-11-30 Thread SmileyChris 
On Dec 1, 6:08 am, Luke Plant  wrote:
> > I was with you right up until the equality comparisons (Null ==
> >  Null -> False etc). As noted by Alex, it conflicts with the answer
> >  given by {% ifequal %}; it also differs with Python's behavior.
>
> Yeah, I hadn't thought of that. I don't think it's a case of differing
> with Python's behaviour, as we are making a deliberate choice to
> substitute a missing variable with None (or Null or whatever).  You
> could argue that Python throws a NameError in this case.

Personally, I think that "(not found)" should *not* equal "None", even
if that differs from ifequal.

But whatever, I'm not too worried either way.

--

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.

Re: smart if tag

2009-11-30 Thread Luke Plant 
On Monday 30 November 2009 11:18:13 Russell Keith-Magee wrote:

> > 1) Handling non-existent variables
> > ==
...
> > To do this properly, I think the best approach would be something
> > like "almost three valued logic", which would work as follows:
> >
> > 1) variables that don't exist are represented by Null
> > 2) all operators return either True or False, never Null
> > 3) for 'not', 'and' and 'or', Null effectively acts like False
> >   (so "not Null" == True, "Null or True" == True etc)
> > 4) for all comparison operators, if either argument is Null then
> >   the result is False e.g. "Null == Null" is False
> >   "Null > 1" is False, "Null <= 1" is False, except
> >   "!=", which should return true if either value is Null.
> >
> > Any comments on this approach? I've started to implement this,
> > and it works OK so far, just needs a bit more work.
> 
> I was with you right up until the equality comparisons (Null ==
>  Null -> False etc). As noted by Alex, it conflicts with the answer
>  given by {% ifequal %}; it also differs with Python's behavior.

Yeah, I hadn't thought of that. I don't think it's a case of differing 
with Python's behaviour, as we are making a deliberate choice to 
substitute a missing variable with None (or Null or whatever).  You 
could argue that Python throws a NameError in this case.

I was trying to think about what is intuitive. e.g. what should happen 
here if foo has not been supplied:

{% if foo < 1 %}
  yes
{% endif %}

I think it is fairly counter-intuitive for a template author that you 
get 'yes' here.  However, on further reflection, the "almost 3 valued 
logic" can be equally counter-intuitive, say with the following case:

{% if foo < 1 %}
  yes
{% else %}
  no 
{% endif %}

It would render 'no' if foo was missing, but it might be quite 
surprising that if you decided to re-arrange the template by inverting 
the logical condition:

{% if foo >= 1 %}
  no
{% else %}
  yes 
{% endif %}

then you get different behaviour. (i.e. 'yes' if foo was missing).

So, I'm *now* suggesting that we convert everything missing to None.

In fact, I've found that doing anything apart from this would be hard.

(Alex, you were right about what Malcolm had done with 
FilterExpression, which resolves the problem with 
TEMPLATE_STRING_IF_INVALID, but this gives me None instead of '', and 
not information about whether the variable actually exists, which is a 
slightly different question. I didn't find this out earlier because 
most of the tests are against the 'IfParser' which is a subclass of 
'TemplateIfParser', and works slightly differently, and because I 
wasn't thinking properly).

Behaviour of 'not' - revisited
==

I think this leaves just the behaviour of 'not'.  It is a bit more 
complicated than Alex suggested — the tests include "if not" and "if 
not not", and with the current behaviour you can do things like "if 
foo and not not" etc.

In fact, you can do it (to some extent) with *any* of the keywords 
e.g. "if and", "if x and or".  But not always e.g. "if not or and x"  
- that, illogically, is a syntax error.

I suspect that allowing everything that was possible before will be 
extremely difficult, because the current behaviour is very badly 
defined for these edge cases — it works just fine if you do sensible 
things like not using variables called 'not', 'and' or 'or', but 
working out the rules for the exceptional cases will be very hard.

I propose a backwards incompatibility here, it looks like this:

 The 'if' tag no longer accepts 'and', 'or' and 'not' as valid 
 variable names.  Previously that was allowed in some cases even 
 though they were normally treated as keywords.  Now, the keyword
 status is always enforced, and code like {% if not %} or {% if and %} 
 will throw a TemplateSyntaxError.

This would now be the only backwards incompatibility (discounting the 
possibility that people were relying on TemplateSyntaxError for things 
that are now legal).

Patch
=

It can be tracked in my 'lp-smartif' branch here:
http://bitbucket.org/spookylukey/django-trunk-lukeplant/

Currently various tests about 'not' are failing, and docs are not 
written.

Chris: in the course of my attempted 'Null' behaviour changes, I 
changed your implementation slightly — I added explicit classes for 
Less, LessOrEqual, NotEqual and Not, pulling out the 'negate' 
behaviour from the individual classes (it made implementing the Null 
logic simpler).  Other than that, very little of the parsing has 
changed.  Given that the 'Null' stuff has now been removed, we could 
move back to your way to reduce the code a bit, but I'm not sure it is 
worth it.

Review would be welcome, especially as I'm ill at the moment. I'm only 
coding because the boredom of doing nothing is killing me...

Luke

-- 
"Humiliation: The harder you try, the dumber you look." 
(despair.com)

Luke Plant || http://lukeplant.me.uk/

--

You received this

Re: smart if tag

2009-11-30 Thread Russell Keith-Magee 
On Sun, Nov 29, 2009 at 1:40 AM, Luke Plant  wrote:
> Hi all,
>
> I started work replacing Django's if with the "smart-if" template tag
> by Chris Beaven ( http://www.djangosnippets.org/snippets/1350/ )
>
> Of course, it is not as simple as it looks...
>
> 4 issues:
>
> 1) Handling non-existent variables
> ==
>
> The current smart-if does not handle the case of non-existent
> variables like Django does.  (For example, something like
>
>  {% if foo or bar %}
>
> when one of foo or bar is not defined in the context.)
>
> To do this properly, I think the best approach would be something like
> "almost three valued logic", which would work as follows:
>
> 1) variables that don't exist are represented by Null
> 2) all operators return either True or False, never Null
> 3) for 'not', 'and' and 'or', Null effectively acts like False
>   (so "not Null" == True, "Null or True" == True etc)
> 4) for all comparison operators, if either argument is Null then
>   the result is False e.g. "Null == Null" is False
>   "Null > 1" is False, "Null <= 1" is False, except
>   "!=", which should return true if either value is Null.
>
> Any comments on this approach? I've started to implement this, and it
> works OK so far, just needs a bit more work.

I was with you right up until the equality comparisons (Null == Null
-> False etc). As noted by Alex, it conflicts with the answer given by
{% ifequal %}; it also differs with Python's behavior.

> 2) TEMPLATE_STRING_IF_INVALID breaks everything
> ===
>
> The smart 'if' tag allows for filters:
>
> {% if articles|length >= 5 %}
>
> To achieve this, it puts its arguments through FilterExpression.  If
> settings.TEMPLATE_STRING_IS_INVALID is not an empty string, this means
> that:
>
> {% if foo %}blah{% endif %}
>
> will render 'blah' instead of '' if 'foo' is not in the context.  This
> causes test failures at the moment.  We could change the tests, but
> that is a backwards incompatibility, and it highlights a fundamental
> change in the behaviour of {% if %}, which basically makes
> TEMPLATE_STRING_IF_INVALID useless for even its stated purpose of
> debugging, because the logic of a template will now be changed.
>
> Personally, I care nothing for TEMPLATE_STRING_IF_INVALID, as there
> are other things that it breaks, and I never use it. But obviously I
> can't just think of myself here.
>
> If there was an easy solution that didn't change the behaviour of the
> if tag in this case that would be great, but I can't see one.

The resolve call on FilterExpression takes an "ignore_failures"
argument that exists for exactly this purpose - contrib.comments uses
this setting to ensure that generic content type lookups don't end up
with spurious data. I suspect the same argument should be usable here,
too.

> 3) Behaviour of 'x and b or y'
> ==
>
> Previously this was a syntax error.  In Chris's code, it is fine and
> works like Python. I would vote for keeping the smart-if as it is, as
> I don't see the value of complicating the implementation to disallow
> something that could be useful sometimes.

I agree. While I still think having excessive logic in the template is
a bad idea, I don't see too much harm in providing the capability on
the off chance it is legitimately useful. This is double so given that
preventing complex logic will complicate the implementation, and
almost inevitably cause grief from those users that don't understand
why the limitation is in place.

The kink in this particular plan is that it will require clearly
documenting the order of evaluation of logical operators - especially
since parentheses aren't available for explicit clarification.

> 4) Behaviour of 'not'
> =
> Current Django will interpret 'not' as a variable if there is no
> ambiguity e.g.
>
>   {% if not %}blah{% endif %}
>
> This is not documented, but the behaviour is specified in detail in
> the tests, so changing it would be a backwards incompatibility. The
> smart-if is different (the above is a syntax error), and I suspect
> that making the smart-if behave like current Django could complicate
> things a lot.
>
> IMO, current Django behaviour here is complete misfeature! I don't
> know of any other language where keywords can be treated as variables
> if the keyword doesn't make sense in that position...

I completely agree that this is a nasty wart, but I'm not completely
convinced that it should count as a backwards incompatible change. We
don't document this particular quirk of the if tag, and I don't think
we've ever claimed that the test suite is the ultimate arbiter of
correctness. After all, the test suite can have errors, and the wrong
behavior is still wrong, even if the wrongness is tested and
validated.

Personally, I think this falls into the same category as the
introduction of the 'permanent' argument on the redirect_to generic
view.

Re: smart if tag

2009-11-29 Thread SmileyChris 
On Nov 29, 6:40 am, Luke Plant  wrote:
> Hi all,
>
> I started work replacing Django's if with the "smart-if" template tag
> by Chris Beaven (http://www.djangosnippets.org/snippets/1350/)

Neat! I'm assuming you'll be posting this to your bitbucket soon? :)

> 1) Handling non-existent variables
> 2) TEMPLATE_STRING_IF_INVALID breaks everything

Yep, this is a bit of a problem which I've been thinking about
recently (encountered it for a project).

My (non-implemented) solution to both of these is to use a NotFound
object. I just coded one up: http://gist.github.com/244861
This could actually be a much better return value than a plain
TEMPLATE_STRING_IF_INVALID string in general.

> 3) Behaviour of 'x and b or y'

Like I commented in the snippet, I don't think it's worth obfuscating
code to limit this behaviour.

> 4) Behaviour of 'not'

Bleh, whatever. If we need it for backwards compatibility, it's not
hard to implement.

--

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.