On Wed, Mar 24, 2010 at 3:06 PM, Gary Bernhardt <[email protected]>wrote:
> On Wed, Mar 24, 2010 at 2:42 PM, Ingy dot Net <[email protected]> wrote: > > Thanks fellas, > > > > Those are fine ideas, but I guess what I was really hoping for was not so > > much help with that specific question, but some review of my newbie > python > > module code. > > > > In many ways, Python is very straightforward, but in other aspects I > spent > > hours looking for the right way to do something. Often times I wasn't > sure I > > got it right. I did read PEP-0008 and an old OSCON talk called "Code Like > a > > Pythonista". Still it would be great to have some old hands give it a > quick > > look. It's not that much code. > Hi Gary, Thanks for doing this. It was just the kind of thing I was hoping for. :) I just pushed out http://pypi.python.org/pypi/pyplay/0.5 Here are some small-scale things that stand out in a skim, in order as > I read through the file: > > - There are no tests for this code! ;) > Fixed! But see bottom of msg... - Don't call modules.__reversed__; magic methods are magic for a > reason and should very rarely be called directly. You want the > reversed() built-in instead. > Fixed! - "del"ing local variables at the end of a function doesn't do > anything; it just destroyed the local reference, which would've been > destroyed when the function returned anyway. > Actually it does do something in this case. When I ran the command: >>> y(globals()) inside pyplay, it showed the 'command' variable, which is an internal detail that the user doesn't need to know about. I get your point in general though. - Short-circuiting "and"/"or" tricks are dangerous – in Python, there > many false values (0, [], {}, "", etc.) that can trip you up. Instead, > use a proper if statement or the ternary syntax: "val1 if condition > else val2". > Fixed! I left in dir = ENV_CONFIG_DIR or LOCAL_CONFIG_DIR or HOME_CONFIG_DIR but I just set those variables, so probably no surprises. I'm glad to know about this ternary syntax. I didn't know it existed. I assumed it didn't because the familiar ?/: operators were not there. Pythons ternary syntax reads nicely. I'll have to see how it looks when nested. Let's try... if foo: x = 1 elsif bar: x = 2 else: x = 3 In Perl it would be something like: x = foo ? 1 : bar ? 2 : 3 I Python, I think it's: x = 1 if foo else 2 if bar else 3 Wow, pretty nice so far. I hate multiple assignment statements, and have been known to nest ternaries very deep to avoid them. I guess I'll see how it goes. - Instead of asking a dictionary for a key and catching the KeyError > (four lines), consider my_dict.get(key, default_value) (one line). > Super! fixed. - Square the comments about and/or short circuiting when both "and" > and "or" are involved. > > I hope this is useful. :) > Very. More please! ---- OK, about the testing. I fixed all your findings in just a few minutes but then spent the rest of the night trying to figure out python testing. I wanted to make it so that the tests would run with `make test`. I have a tiny Makefile that proxies targets to setup.py, so that would also be `python setup.py test`. It drove me to fits, and I would love some pointers here. You can see how I did it, but it's not particularly pretty. I didn't even use unittest or py.test or nose yet. Just an assert. I couldn't figure out how to invoke the framework tests from setup.py. And adding the 'test' command to setup.py took a lot of code. Thought it would be zero or one lines. :) Cheers, Ingy > -- > Gary > http://blog.extracheese.org >
