?

Log in

No account? Create an account
Python - brad's life [entries|archive|friends|userinfo]
Brad Fitzpatrick

[ website | bradfitz.com ]
[ userinfo | livejournal userinfo ]
[ archive | journal archive ]

Python [Aug. 15th, 2007|02:41 am]
Brad Fitzpatrick
[Tags|, , ]

Please criticize my first Python program.

I really should read a book. What's everybody's favorites?
LinkReply

Comments:
[User Picture]From: dimrub
2007-08-15 10:24 am (UTC)
You will do fine (actually, you do do fine) with the stuff on the Python's site. Being a partisan, I will however recommend the "Python's cookbook", for which a friend of mine wrote a chapter or two.
(Reply) (Thread)
[User Picture]From: ciphergoth
2007-08-15 10:29 am (UTC)
The only thing I can see to criticize at first glance is that it might be worth checking whether the exception is one you expect before ignoring it.

Also, if this is Python 2.5, look at using "with" to simplify your use of files:

http://docs.python.org/whatsnew/pep-343.html
(Reply) (Thread)
From: ext_51207
2007-08-15 10:40 am (UTC)
Dive into Python (http://www.diveintopython.org/) by Mark Pilgrim is a must read!
I also recommend checking out his blog, good stuff.
(Reply) (Thread)
From: node
2007-08-15 10:54 am (UTC)
file is a builtin, like open. You probably shouldn't use it as a variable for the same reason you're not using list as a variable.

You can iterate over dicts with for k, v in data.items(). You could del data[k] after the unlink.
(Reply) (Thread)
From: node
2007-08-15 10:55 am (UTC)

also, try using

pylint (should be in Debian).
(Reply) (Thread)
[User Picture]From: ghewgill
2007-08-15 11:00 am (UTC)

If you're iterating through a collection and deleting stuff, you ran into the obvious problem. The pythonic solution to that is something like:

for k in data.keys():
    if condition:
        del data[k]

That way you don't have to fiddle with todel.

(Reply) (Thread)
From: trs80 [typekey.com]
2007-08-15 01:35 pm (UTC)
for k in data[:]:
is another way I saw recently.

Back to the program, the biggest problem I have with mailman (although it's probably true of most mailing list managers) is that there's not much of a public API apart from a few command-line programs. Of course, I just went at looked at the docs, and bin/withlist provides exactly what I want.
(Reply) (Parent) (Thread)
[User Picture]From: brad
2007-08-15 07:26 pm (UTC)
Ah, thanks for the "withlist" pointer. That'll be more useful as I learn more Python.
(Reply) (Parent) (Thread)
[User Picture]From: decklin
2007-08-15 05:48 pm (UTC)
Yeah. Perhaps the clearest way to communicate the intention around this part (i.e., the int test) is to be even more explicit about coming up with the list first:

keys = [k for k in data if isinstance(k, int)]
for k in keys:
  # don't do the if-continue
  # etc...

Don't ask me about isinstance vs. types.Whatever though; this is just how I tend to say it. I'm sure other people can offer some strong opinions.

It's worth noting that the objections presented here apply equally to either approach. It would be better to think about what's in data.keys() that's not an int and handle any exceptions that could possibly come up. This, of course, means that the loop above is moot :-) (this is a good thing; just calling data.keys() is more succinct, even if it doesn't have a big red sign that says THIS IS NOT AN ITERATOR to newbies.) I don't know why Mailman's data structures would lead to this problem, but they do look rather weird with the apparent tuples and crap (/me shrugs). My style in those cases is to always do a multiple assignment even if i'm going to throw several of the values away. I hate looking at indices.
(Reply) (Parent) (Thread)
From: (Anonymous)
2007-08-15 11:12 am (UTC)

Python Links...

This is funny. Mostly because it's true:

http://dis.4chan.org/read/prog/1180084983/

This has nice coverage of Python's functional support:

http://gnosis.cx/TPiP/

Some interesting content in these videos:

http://video.google.com/videoplay?docid=-3035093035748181693
http://video.google.com/videoplay?docid=-288473283307306160

-ryan
www.asciiarmor.com
(Reply) (Thread)
From: amoffat
2007-08-15 11:15 am (UTC)
list comprehensions are (trivially) cooler than the filter builtin

lists = filter(os.path.isdir, os.listdir("."))
lists = [d for d in os.listdir(".") if os.path.isdir(d)]

might want to throw your top-level code in a def main() and add a if __name__ == '__main__': main() to the top level. makes for easier organization (and makes sure that if your code is imported, it isn't run automatically, not that it matters here in this instance)

you can do type(k) != int, because int is a type (along with str, float, etc). saves an import and some typing

other than that, not bad for a first python program. maybe you could critique my first perl program when i get around to writing it :)
(Reply) (Thread)
From: (Anonymous)
2007-08-15 12:04 pm (UTC)

pythonic style

If you want some advice regarding the "pythonic" style you can take a look here:

http://www.python.org/dev/peps/pep-0008/
http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html

Regarding your first program, I suggest you use isinstance instead of checking directly the type (see pep-8 also).

>>> type(1) != types.IntType
False
>>> not isinstance(1, int)
False

ciphergoth is right regarding the with statement and, as ghewgill said, you can improve the way you're working with the data dictionary. You can modify it during the iteration if you use the keys() or items() methods (just be sure you're not using one of the iter* equivalents). I would probably use data.items() instead of data.keys() so you have direct access to the key and it's value.

Favorites books? Dive Into Python it's free and great (only lack is that it covers python 2.2 IIRC), Learning Python and Python in a Nutshell have been updated to 2.5 I think. The python tutorial is also a great source.

Great to see you stepping on python. :-)
(Reply) (Thread)
From: (Anonymous)
2007-08-15 12:13 pm (UTC)
The Laughing Jesus by Timothy Freke!
(Reply) (Thread)
From: seeds_of_peace
2007-08-15 12:14 pm (UTC)
The Laughing Jesus by Timothy Freke!
(Reply) (Thread)
[User Picture]From: gnuvince
2007-08-15 12:40 pm (UTC)
Dive Into Python is generally recommended for people who know how to program and want to learn Python without having to read about what a variable or a string is.
(Reply) (Thread)
[User Picture]From: muerte
2007-08-15 01:47 pm (UTC)
(Reply) (Thread)
[User Picture]From: codetoad
2007-08-15 03:09 pm (UTC)
Quick things I noticed:

Docstrings are the convention for whole-program and whole-function notes. This means you want:

#!/usr/bin/python

"""
Remove moderation items from GNU Mailman's pending queue.
...
"""


Also, as kinda noted below, if you do:

if __name__ == '__main__':
for lst in lists:
    print "Cleaning list %s ..." % lst
    clean_list(lst)


you'll be able to use the program as a module ( without executing the main stuff automatically.
(Reply) (Thread)
[User Picture]From: avatraxiom
2007-08-15 03:27 pm (UTC)
Generally you want to use instanceof instead of comparing types directly. I'm pretty sure this applies to built-in types as well, as it's a problem most commonly run into with string types (where ascii strings have one type, and unicode strings have a "unicode" type, but they both inherit from basestring). It's like using ->isa instead of "blessed($ref) eq".

The filter could alternately have been written as a list comprehension, but it's actually probably clearer as a filter (which is kind of unusual). If you're curious, I think the list comprehension would have been:

[x for x in os.listdir(".") if os.path.isdir(x)]

-Max
(Reply) (Thread)
[User Picture]From: burritob
2007-08-15 03:28 pm (UTC)

Python books

Firstly, I'd recommend avoiding O'Reilly's "Learning Python" and "Programming Python" - they're vastly inferior to their Perl counterparts.

Mark Pilgrim's Dive Into Python is worth a look.

I'm also a big fan of the O'Reilly Python Cookbook.
(Reply) (Thread)
From: evan
2007-08-15 03:56 pm (UTC)
def clean_list (lst):
no space before parens

main code usually all goes together at the bottom (makes it more clear where "lists" comes from in the third-to-last line)

agree with the above that you shouldn't ignore all exceptions -- can mask e.g. typos

i'd probably do something like
path = '%s/request.pck' % lst
as the first line of the function so you don't have to repeat it (can do "path + '.new'" at the end)
(Reply) (Thread)
From: evan
2007-08-15 03:59 pm (UTC)

books

I think I just read the tutorials on their site, but I'm no expert. diveintopython is good and freely available online.
(Reply) (Thread)
[User Picture]From: markpasc
2007-08-15 04:41 pm (UTC)

Re: books

(Reply) (Parent) (Thread)
[User Picture]From: dnab
2007-08-15 05:12 pm (UTC)

Re: books

+2
(Reply) (Parent) (Thread)
[User Picture]From: kvance
2007-08-15 04:21 pm (UTC)
O'Reilly's Python Cookbook is a pretty good resource for seeing the pythonic way to do a lot of stuff.
(Reply) (Thread)
[User Picture]From: beckyzoole
2007-08-15 04:46 pm (UTC)
Heh heh -- I first read the post as two non-connected topics, and was about to suggest The Curious Incident of the Dog in the Night-time....
(Reply) (Parent) (Thread)
[User Picture]From: mart
2007-08-15 05:11 pm (UTC)

Python looks horrible :(

(Reply) (Thread)
[User Picture]From: fallenpegasus
2007-08-15 06:50 pm (UTC)
Python is beautiful.

I've learned thru hard and horrible experience that I cannot trust programmers to make their physical indentation match the logical indentation. C, C++, and Ada programs that are all left justified, or even worse, randomly indented. Shudder.

So something that forces them to do it right or at least better, I'm all for.
(Reply) (Parent) (Thread)
From: (Anonymous)
2007-08-15 05:38 pm (UTC)
Looks pretty good. Here's my changes:

http://rafb.net/p/MXRxRX27.html

HTH.
(Reply) (Thread)
[User Picture]From: decklin
2007-08-15 05:51 pm (UTC)
Also:

data = cPickle.load(file('%s/request.pck' % lst, 'rb'))

open() is deprecated; you should use the builtin file() constructor rather than creating a variable called "file" :-)

Written as above, the object is anonymous and thus garbage collected and thus close() is automatically called for you. This is maybe a matter of taste. I go for one-liners when I can.
(Reply) (Thread)
From: (Anonymous)
2007-08-15 07:03 pm (UTC)

open() is not deprecated

open() is not deprecated:
When opening a file, it's preferable to use open() instead of invoking this constructor directly. file is more suited to type testing (for example, writing "isinstance(f, file)").
(source: http://docs.python.org/lib/built-in-funcs.html (http://docs.python.org/lib/built-in-funcs.html))

--
dsp
(Reply) (Parent) (Thread)
[User Picture]From: decklin
2007-08-15 07:29 pm (UTC)

Re: open() is not deprecated

Oh, interesting, thank you. It appears that this is a change in 2.5 from 2.4. Do you have a link to a PEP that would shed some light on the rationale?

Personally, I have gotten used to the old new way of doing it, and open() feels a bit clunkily procedural to me. But, as I implied, there's no accounting for taste. :-)
(Reply) (Parent) (Thread)
[User Picture]From: decklin
2007-08-15 05:59 pm (UTC)
And furthermore:

Reader's Block, David Markson.
(Reply) (Thread)
[User Picture]From: decklin
2007-08-15 07:38 pm (UTC)
Damn it, no one thought this was funny. :-)
(Reply) (Parent) (Thread)
[User Picture]From: jtrevino
2007-08-15 06:07 pm (UTC)
I've been doing some Python at my current job, so I pinged my friend Guido with the same Python book question a couple of months back. Guido's online tutorial is pretty good: http://docs.python.org/tut/ . After that, Guido recommends the following:

...Really good however are: David Beazley, Python Essential Reference (3rd ed!);
Dive Into Python by Mark Pilgrim (also on the web I believe); and two
books by Alex Martelli (also a Googler): Python in a Nutshell and
Python Cookbook. Theer's more on python.org if you search around a bit
for introductions or books.

(Reply) (Thread)
[User Picture]From: mtbg
2007-08-15 06:40 pm (UTC)
Previous comments that I give a +1: use isinstance, no space between function name and lparen, wrap top-level executable statements in main(), unqualified except statements are the DEVIL, not overwriting the file() builtin (I remember being very confused after accidentally assigning to 'sum' one time).

For portability reasons, the shebang in Python scripts is typically '#!/usr/bin/env python'.

If you import os, you don't have to separately import os.path.

I would probably go with the 'for k in data.keys():' approach. I keep wishing that Python allowed 'for x in foo if predicate(x):' (which I frequently approximate with 'for x in [x for x in foo if predicate(x)]:', which feels sort of gross).

Props for building strings with % instead of concatenation.
(Reply) (Thread)
[User Picture]From: decklin
2007-08-15 07:35 pm (UTC)
Yeah, that truly annoys me. Sometimes what i really *want* to say is for x in [x for x in blah]... but it just looks so gawky. I love list comprehensions in general but it's a shame that the filter() approach wasn't well-loved enough to get nicer syntax...

(As the length of any Python thread increases, the probability that someone will mention getting rid of lambda approaches 1.)
(Reply) (Parent) (Thread)
(Deleted comment)