Re: Comments merging, pass 2

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.

On Sat, 11 Dec 2004 02:22:17 +0100, Steven Armstrong <[email protected]> wrote:
> Another patch for comments.py
> 
> It adds the following:
> 
> 1. property: comment_fixlink
> if True: fixes urls that are missing the protocol
>    e.g. www.example.com becomes http://www.example.com
> if False: drops urls without protocols
>    e.g. www.example.com becomes ''
> if not set: does nothing
> 

That's a good idea, I like that.

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

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

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 = ''

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.

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

> >         if not "://" in url:
> >             url = (config['comment_fixlink'] and ["http://%s" % url] or [''])[0]

same comment as above

> >     else:
> >         # raise KeyError to keep Bill's build_preview_comment happy
> >         if raiseError:
> >             raise KeyError("url")
> >     return url
> >
> >
> 478c533
> <
> ---
> >
> 486,488c541,543
> <         # Check if the form has a URL
> <         url = (form.has_key('url') and [form['url'].value] or [''])[0]

here's that formula again

> <
> ---
> >         # Check if the form has a URL and if it's absolute
> >         url = _fixlink(config, form)
> >
> 497c552
> <
> ---
> >
> 522a578
> >     form = request.getHttp()['form']
> 523a580
> >
> 526c583,585
> <             and not entry.has_key("nocomments"):
> ---
> >             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'))

except that you made it look really complicated. I think; I'm still
not 100% sure.

> 534c593
> < def build_preview_comment(form, entry):
> ---
> > def build_preview_comment(request, entry):
> 536a596,597
> >     form = request.getHttp()['form']
> >     config = request.getConfiguration()
> 542,543c603,607
> <         c['cmt_link'] = form['url'].value
> <         c['cmt_item'] = sanitize(form['body'].value)
> ---
> >         # set raiseError=False to make cmt_link optional
> >         c['cmt_link'] = _fixlink(config, form, raiseError=True)
> >         # sanitize only once
> >         description = sanitize(form['body'].value)
> >         c['cmt_item'] = description
> 545c609
> <         c['cmt_description'] = sanitize(form['body'].value)
> ---
> >         c['cmt_description'] = description

Fair enough; I didn't know what 'cmt_link' was, so I didn't want to
set it into stone.

> 547a612
> >
> 549a615,623
> >
> >     # added new variable cmt_author_link.
> >     if c.has_key('cmt_author') and c.has_key('cmt_link'):
> >         if c['cmt_link'] != "":
> >             entry['cmt_url'] = c['cmt_link'] # keep Steven's template happy
> >             c['cmt_author_link'] = '<a href="%s">%s</a>' % (c['cmt_link'], c['cmt_author'])
> >         else:
> >             c['cmt_author_link'] = c['cmt_author']
> >
> 550a625,628
> >     # 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.

> 552a631
> >
> 559a639
> >
> 561a642,643
> >             and (config.has_key('comment_trigger') and \
> >                 [form.has_key(config.get('comment_trigger'))] or [True])[0] \

and it makes one more appearance. Again,
[form.has_key(config.get(...))] always evaluates to True, and the
[True] never gets hit.

> 568c650
> <             if form.has_key('preview')\
> ---
> >             if form.has_key('preview') \
> 570c652
> <                 com = build_preview_comment(form, entry)
> ---
> >                 com = build_preview_comment(request, entry)
> 576a659,660
> >
> >
> 
> 
> 

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/