Export to GitHub

neap - issue #6

[PATCH] Suggested changes to bring neap into line with common Python coding practices


Posted on Jun 4, 2010 by Happy Bird

While looking through your code, I noticed that you seem to be unaware of many of the de facto standard stylistic conventions and standard library features.

The attached patch against SVN trunk doesn't fix everything, but it makes the changes I thought you'd be more receptive to: 0. My vim config automatically cleaned up some trailing spaces. 1. The "doc comment" at the top of the file should be a module docstring. 2. Most Python style guides recommend grouping stdlib, 3rd-party, and local imports separately. 3. Module metadata fields have names that documentation and static analysis tools expect and "just work" with (author_, __version, license) and name is already taken, so I used appname 4. Constant data (especially stuff with different indentation semantics) should go at the top of the file. 5. While it's actually a bug rather than a stylistic thing, you accidentally forgot to initialize self.grid, instead saying "grid = []". (PyChecker caught it) 6. "map" is a built-in function. I renamed your variable to "colormap". 7. Returning -1 to indicate failure is bad form. Return "None" or "False" instead as appropriate so you can just say "if foo.method():" or "if not foo.method():" or "if foo.method() is None:" as appropriate. 8. Class names should begin with a capital letter. 9. Old-style classes "class Neap:" are deprecated and have been removed in Python 3.0. (Just fixing this removed a ton of spurious PyChecker warnings)

Other things I didn't change which I could if you're willing: 1. You set self.menu (and don't initialize it in init) but never use it. 2. Rather than Neap.get and Neap.set, it'd probably be better to use Python's internal getter/setter syntax and just use neap['foo'] = 'bar' or baz = neap['quux'] (or spam = self['eggs'] for accessing data from methods) 3. Unless your goal is to suppress tracebacks, I see no purpose to catching errors in code outside the Glib event loop, printing the message, and then exiting. (Python's standard operating procedure is to print the traceback followed by the message and then exit. You only get errors not terminating the program once gtk.main() takes over) 4. You've got a partially-written ~/.config config writer and you're not using it. If you can tell me what your intent was, I can complete the process and switch neap over (including code to move the rcfile from the old location) if you'd like. (Also, it's not necessarily ~/.config. That's only if $XDG_CONFIG_DIR is unset) 5. Have you considered using the RawConfigParser class in the Python standard library for reading Neap configs instead? (Aside from using sections, it's the same syntax you're already using) 6. I'm pretty sure at least some of the stuff you're using python-xlib for can be done using just X11-specific calls in PyGTK. It was originally a quick hack, so I'm not proud of the code, but you should take a look at the get_active_window() method of my QuickTile script at http://github.com/ssokolow/quicktile/blob/master/quicktile.py#L294 7. I could be wrong, but I think there's an off-by-one error in get_screen(). I noticed that, when I was clicking the tiny boxes in a 22px switcher with 3x2 desktops, the active regions seemed offset from the drawn boxes. 8. Many Python coders (myself included) and many style guides (including PEP 8 (Guido van Rossum's style guide) and the Google Python style guide) specify indentation with 4 spaces rather than one tab character. (Because 8 spaces is too much and changing tabsize is guaranteed to cause problems eventually)

My advice is to not release a new version immediately. Instead, let me know if there are any more changes you'd accept patches for and I'll get to work on them.

Attachments

Comment #1

Posted on Jun 4, 2010 by Swift Dog

Hehe, you caught me, my Python is rather bad. This is my second Python program (the first one being a Sudoku solver to get acquainted with the syntax). Also, I'm completely new to X11 stuff.

I love the changes in your patch and will happily apply them to the next release, see SVN rev46. As for the other things you mentioned: 1. I unintentionally didn't remove all references to self.menu; the right-click menu used to be saved there, but now it's rebuilt on demand, so self.menu is superfluous. 2. never heard about Python setters/getters, but it sounds like a good idea to use them 3. my goal was to make it idiot-proof, but from a development point of view, it might be better to get the whole traceback 4. My intention was to save the config in ~/.config/neap/neaprc if ~/.config exists (or, after you told me, in ~/$XDG_CONFIG_DIR/neap/neaprc if $XDG_CONFIG_DIR is set), otherwise save it to ~/.neaprc 5. I was not aware of the existence of the RawConfigParser, and it's sure better to use this instead of my handcrafted code. 6. I'm unsure about this, but it may be less error-prone to use PyGTK instead of python-xlib wherever possible. 7. Completely offset or only on the right side of each box? Because if I remember correctly, the space between two boxes actually belongs to the active region of the left box (in order to not waste "active region" space) 8. Being mainly a C programmer, it's hard for me to abandon tabs, and I think it's a horrible idea to use only 4 spaces for indentation, but it seems that I will have to trust your Python experience here

So in short, I like all of your proposals. If you're willing to work on these issues, I will enjoy incorporating your patches. Moreover, if you're also willing to keep the ChangeLog up to date, I can give you SVN write access.

Comment #2

Posted on Jun 4, 2010 by Happy Bird

I see no problem keeping the ChangeLog up to date, so let's go for the SVN access for convenience. We can leave this bug open as a reminder until I get the second group of issues fixed.

As for the syntax you used, your style does say "C programmer" now that I think about it. (A general tendency to code well but duplicate code in certain places because you don't know about some of the more dynamic aspects of Python, imperatively-structured methods, signed integer return values to indicate success/failure, etc.)

There are some suggestions I didn't make which I'll have to share with you. They're more stylistic choices, but I tend to prefer a somewhat more functional coding style than C allows and I noticed some places where, depending on how easily you grasp them, they could make the code cleaner.

Comment #3

Posted on Jun 4, 2010 by Swift Dog

You are now a "project committer", which enables you to do just about everything useful, mainly full SVN access, wiki writes and downloads management.

I'm excited to hear about your suggestions regarding style. When I look at neap, I sometimes have the feeling that Python allows for a more elegant approach and that this isn't it. I have no problems with a more functional coding style. In fact, I do have some experience with functional programming languages such as ML and Scheme, but this was years ago. However, the basic concepts are completely clear to me.

I may add that I'm happy to see you join this little project. I'm expecting to further advance my Python skills just by looking at the way you handle certain problems. So far, I have already learned quite a bit.

Comment #4

Posted on Jun 5, 2010 by Happy Bird

Whoa. Quite the surprise to see the panel of options that jumps out at me when I focus the comment box now. :)

Anyway, I'll probably first start on your config system. Python's syntax and feature set are designed around being able to write simple, concise, pretty code without preventing future extensibility and the way neap does things right now smacks of the "pre-plan for future extensibility" habits necessary for C but excessive to the point of almost being counter-productive for Python.

Here are a couple of examples of how Python lets you redefine behaviour while maintaining API compatibility:

http://snippets.dzone.com/posts/show/954 http://diveintopython.org/object_oriented_framework/special_class_methods.html

I'll push a cleaned up config system to the repo once I finish wrapping my mind around the code you've got now. Do you have an IM account we could use to discuss the changes with a lower latency than e-mail or this issue?

Comment #5

Posted on Nov 1, 2010 by Happy Horse

(No comment was entered for this change.)

Status: Fixed

Labels:
Type-Defect Priority-Medium