[Pythonmac-SIG] Please critique my first appscript - a fix for Palm sync of iCal Tasks (for kGTD)

[Nicholas Riley]

These are (mostly) all issues of personal opinion, but anyway...

On Tue, Aug 29, 2006 at 12:49:44AM -0400, Marcin Komorowski wrote:

>#!/usr/bin/env /usr/bin/pythonw

There's no point of using env if you specify an absolute path.
"#!/usr/bin/env pythonw" is probably what you want.

> def myDebug( txt ):
>     if txt == None:
>         sys.stderr.flush()
>     else:
>         sys.stderr.write( txt )

stderr should be unbuffered by default; you shouldn't need to flush it
at all.  Also, if you've got a sentinel value, try representing it
with a default/optional argument, e.g.:

	def myDebug(txt, flush=False):

or:

	def myDebug(txt=None):

> def getTaskList():
>     appCal = app( 'iCal' )

Use a bundle ID or creator to refer to iCal, in case it is renamed.

>     cal_todos = appCal.calendars.filter(its.name !=  
> '').todos.properties()
>     tasks = [ todo  for cals in cal_todos  for todo in cals]

Consider being more consistent about spacing - see PEP 8 for one
possible coding style.  Also, wow, I never noticed how backwards
reading nested list comprehensions is - yuck (Python problem, not
yours...).

>     # Iterate flattened list, builing a local dictionary of  
> dictionaries, by uid
>     tasks_data = {}
>     for task in tasks:
>         if task[ k.class__ ] == k.todo:

Why do you need to do this?  It seems if you ask for todos, all you
get is todos, right? :) If you're working around some iCal bug it
makes sense to comment it.

>             uid = task[ k.uid ]
>             tasks_data[uid] = task;
>             myDebug('.')
>             myDebug(None)

You could do the above with a generator expression, e.g:

tasks_data = dict((todo[k.uid], todo) for todos in cal_todos for todo in todos
		  if todo[k.class__] == k.todo)

(this should be fast enough; I can't imagine it'd be worth displaying progress)

> k_decode = {
>         k.priority      : "k.priority",
>         k.due_date      : "k.due_date",
>         k.completion_date : "k.completion_date",
>         k.description   : "k.description",
>         k.url           : "k.url",
>         k.uid           : "k.uid",
>         k.summary       : "k.summary",
>         k.class__       : "k.class__"
>     }

There's no need to do this; you can just print k.priority, etc.,
directly.

> def diffTaskLists( task_list1, task_list2 ):
>     diff_list = []
>     for uid in [ id  for id in task_list1.keys()  if  
> task_list2.has_key( id ) ]:

You can use 'id in task_list2' instead of 'task_list2.has_key(id)'.
For clarity (if not necessarily speed) you might consider using the
set data type instead.

>         t1 = task_list1[uid]
>         t2 = task_list2[uid]
>         found_difference = False
>         for field in [ k.priority, k.due_date, k.completion_date,  
> k.description, k.url, k.summary ]:
>             if t1[field] != t2[field]:
>                 if found_difference == False:

I generally find it easier to read "if not found_difference".

>                     myDebug( "  task ID %s:\n" % str( uid ) )

You don't need to use 'str', that's what %s does already.  And don't
be afraid of the print statement, e.g.:

	print >> sys.stderr, '  task ID %s:' % uid

>                 myDebug( "    field %s: " % k_decode[field] );
>                 try:
>                     myDebug( "t1 = '%s' t2 = '%s'" % ( t1[field], t2 
> [field] ) )
>                 except:
>                     pass

Eww.  If there are problems converting the fields to a string, you
should address them (or report a bug in appscript or iCal...)  At the
very list, make your except statement more specific so it doesn't mask
other exceptions.

>                 myDebug( "\n" )
>                 found_difference = True
>                 break

Huh, what's the point of checking 'found_difference' then, if you
break immediately after setting it to true?  Looks like you can get
rid of it completely.

> def updateTimeStamp( task_uid_list, timestamp ):
>     myDebug( "Setting new timestamp for task UIDs %s\n" % repr 
> ( task_uid_list ) )

You can use '%r' % task_uid_list to get the repr.

> def usage():
>     print "USAGE: " + sys.argv[0] + " save|check"

It's a bit more efficient (and clearer, IMO) to use commas to give
print multiple arguments rathern than using + to concatenate strings.

> def main(mode):
>     if mode != 'save' and mode != 'check':

	if mode not in ('save', check'):

>     pp = pprint.PrettyPrinter(indent=4)

Since you only use this once, it makes sense to move it closer to its
definition and/or combine the creation and invocation.

-- 
Nicholas Riley <njriley at uiuc.edu> | <http://www.uiuc.edu/ph/www/njriley>