Re: Comments merging, pass 2

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.

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

> <snip>Everything in here was fine with me</snip>
> 
>>>def _fixlink(config, form, raiseError=False):
>>>    """
>>>    If the config property comment_fixlink is set to True:
>>>    makes sure that the url is absolute (includes the protocol).
>>>    If it's set to False, drops url's without protocol.
>>>    Does nothing otherwise.
>>>
>>>    @param config: pyblosxom config dict
>>>    @type config: C{dict}
>>>    @param form: dict containig the GET/POST form fields
>>>    @type form: C{dict}
>>>    @return: the fixed url
>>>    @rtype boolean: C{str}
>>>    """
>>>    url = (form.has_key('url') and [form['url'].value] or [''])[0]
> 

I actually copied this line of code from the original comments impl. :-)))

> 
> Here I think is the formula that is really unpythonic, which you seem
> to use all the time. What does this mean? It takes me a hell of a long
> time to figure it out, and I don't do anything besides write and read
> python all day. Why not write:
> 
> if form.has_key('url'):
>     url = form['url'].value
> else:
>     url = ''
> 

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.

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.

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

<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.
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"):

If someone has not set comment_trigger my part of the statement must 
never return False cause comment_trigger is ment to be an optional property.

config.has_key('comment_trigger')
returns False if comment_trigger is not defined.

form.has_key(config.get('comment_trigger'))
returns False if comment_trigger is not defined.


so the above is again the and-or trick, it's just nested:

var result = (firstCondition)? secondCondition : true

result = (firstCondition and [secondCondition] or [True])[0]

if firstCondition is False result is allwais True.
if firstCondition is True, result is eather True or False depending on 
secondCondition.

This let's me make my check without interfering with the given behaviour.

It's actually the same as:

if config.has_key('comment_trigger'):
   if form.has_key(config.get('comment_trigger')):
     result = True
   else:
     result = False
else:
   result = True

I agree that this would be much more readable.

The only other way I could have coded this is like:

# block 1
comment_trigger_flag = True
if config.has_key('comment_trigger'):
   if not form.has_key(config.get('comment_trigger')):
     comment_trigger_flag = False
# /block 1

if len(renderer.getContent()) == 1 \
   and renderer.flavour.has_key('comment-story') \
   and comment_trigger_flag \
   and not entry.has_key("nocomments"):

But this means that "block 1" is allways executed. Even if in 99.9% of 
the cases it is not relevant.

<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 ... :-))

cheers
Steven


-------------------------------------------------------
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/