Re: Comments merging, pass 2

On Sat, 11 Dec 2004 06:03:01 +0100, Steven Armstrong <[email protected]> wrote:
> Bill Mill wrote:
> > Steven,
> >
> > It seems to me that some of the things you do are rather...
> > unpythonic. That is, not wrong, per se, but they just strike me as the
> > wrong way to do things in python. Obviously, my comments don't count
> > for anything, but they follow anyway. Please don't interpret this as
> > harsh criticism, it's meant to be constructive.
> >
> 
> Hi Bill
> 
> No problem at all. In fact I'm thankfull cause I'm just learning Python.
> 

I'm glad you took it well, especially since I was wrong on a bunch of
it. I knew that would happen - I'm very often wrong when I criticize
someone else.

> >>2. property: comment_trigger
> >>if set, only shows comments if there is eather a form field
> >>or a querystring variable that matches the trigger.
> >>e.g. comment_trigger = "cmt"
> >>-> ?cmt=1 or <input type="hidden" name="cmt" value="1" />
> >>
> >
> > why? just curious.
> >
> 
> If someone looks at a single entry that doesn't nessessarily mean that
> they'll want to read comments or even make a comment. Take for example
> the situation where someone is using the "more" plugin.
> 
> Rendering the comments and especially the comment form with the nospam
> image is rather expensive compared to serving just the entry.
> So my idea was to only do that if it's really required/requested.
> 

Fair enough. Works for me.

> >>>    url = (form.has_key('url') and [form['url'].value] or [''])[0]
> >
> 
> I actually copied this line of code from the original comments impl. :-)))

oh sure, blame it on the other guy :)

> 
> In JavaScript, Java and C you can do neat things like:
> 
> var result = (condition)? "hello" : "world";
> 
> Which returns "hello" if condition is True and "world" otherwise.

And I hate it in those languages too, so I'm admitting a bias on this
issue. Especially in python, where it's not even really in the
language, I don't think it should be used.

> 
> As I learned at
> http://diveintopython.org/power_of_introspection/and_or.html#d0e9975
> 
> result = (condition and ['hello'] or ['world'])[0]
> is the python way to do that safely.
> 
> > It's not slower to execute - in fact it should be faster, since it
> > avoids constructing two lists - and it's just *far* easier to read.
> > The 'and' and 'or' operators should be used with serious caution,
> > IMHO.
> >
> 
> You have a point there with the "creating two lists" argument. Though I
> think that's more an issue in terms of memory usage than execution
> speed. I'll try to make a few tests.
> 

------- time.py --------

import random, timeit

def test1(var1, var2, var3):
    return (var1 and [var2] or [var3])[0]

def test2(var1, var2, var3):
    if var1:
        return var2
    else:
        return var3

def testfunc(func):
    iter = [(a,b,c) for a in (0,1) for b in (0,1) for c in (0,1)]
    for i in iter:
        func(i[0], i[1], i[2])

t1 = timeit.Timer("testfunc(test1)", "from __main__ import testfunc, test1")
t2 = timeit.Timer("testfunc(test2)", "from __main__ import testfunc, test2")

print "t1: %s" % t1.timeit(1000000)
print "t2: %s" % t2.timeit(1000000)

----- time.py -----

:!python test.py
t1: 27.0438070297
t2: 21.8385300636

But, realistically, 6 seconds over a million iterations translates
into a .000006 second difference in execution speed, with integers at
least. I just wanted to write the test for fun :)

Whatever's most readable is what should be used, and I don't find the
and/or version to be more readable; that may just be personal bias.

> >
> >>>    if url != '' and config.has_key('comment_fixlink'):
> >
> >
> > Here, why not just "if url and config.has_key"? url should be either
> > '' or some other string value, and it will automatically evaluate to
> > false if it's ''.
> >
> 
> Didn't know that. Thanks for the input.

no problem. Again, it's a really small nit that I just mentioned
because I was reviewing your code already.

> 
> <snip>
> 
> 
> >>
> >>>            and not entry.has_key("nocomments") \
> >>>            and (config.has_key('comment_trigger') and \
> >>>                [form.has_key(config.get('comment_trigger'))] or [True])[0]:
> >
> >
> > This time, I'm having trouble even figuring out at all what's going on
> > here. I seriously had to take out a pen and paper and diagram it.
> > [True] can never possibly be reached, because [form.has_key(...)]
> > always evaluates to true. Try it out in the interpreter:
> >
> >
> >>if [{}.has_key('doesntexist')]: print 'yup'
> >
> > 'yup'
> >
> > Thus, what you have is exactly equivalent to:
> >
> >
> >>and not entry.has_key("nocomments") \
> >>and config.has_key('comment_trigger' \
> >>and form.has_key(config.get('comment_trigger'))
> >
> 
> No it's not.

You are correct, and I am wrong. I was not seeing things clearly
yesterday afternoon - it all seems really apparent to me now.

> Have a look at the whole statement.
> 
> # original
> if len(renderer.getContent()) == 1 \
>    and renderer.flavour.has_key('comment-story') \
>    and not entry.has_key("nocomments"):
> 
> # new
> if len(renderer.getContent()) == 1 \
>    and renderer.flavour.has_key('comment-story') \
>    and (config.has_key('comment_trigger') and \
>      [form.has_key(config.get('comment_trigger'))] or [True])[0] \
>    and not entry.has_key("nocomments"):
> 

OK, I see what you're saying now. I still have a vague uneasiness
about this statement; it seems to be too complicated for its own good.
But, I can't think of a better way to do it, so I retract any
objection I had to the logic of it.

<snip>clear and patient explanation of how I'm wrong</snip>

> >>>    # store a non-sanitized version of the body to put in the textarea
> >>>    if form.has_key('body'):
> >>>        entry["cmt_body"] = form['body'].value
> >>>
> >
> >
> > Why? My intention was to show the user what their text would look like
> > after sanitization.
> >
> 
> IMHO the value of the textarea should not be changed in any way.
> Take for example someone who allows some funky formatting in the
> textarea (wiki/textile). If he uses preview and then want's to change
> something before submitting he needs the original, non-sanitized,
> version. Otherwise he'll have to start all over.
> 
> Or another example:
> A user has written the comment of his life, it's 1235 words long. He
> spend hours thinking and writing on it.
> 
> Offcourse he wants to preview it, after all it's ment to be a
> masterpiece. So he hits Preview. But ups, he forgot to enter the URL.
> So his masterpiece is gone, instead he sees a message in the textbox
> like "Missing value: url".
> 
> I don't think he would be very happy ... :-))
> 

Yeah, I realized this last night, and added an error variable instead
of changing the comment description. That would really piss me off if
I did that. Good point about the non-sanitization too, I didn't
consider funky syntaxes.

Peace
Bill Mill
bill.mill at gmail.com


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/