Code review comment for lp://staging/~mbudde/earcandy/reorganization

Revision history for this message
David D Lowe (flimm) wrote :

Hello Michael Budde! It seems like we were working on the same bug #379266 in parallel, so a lot of our work is similar, if not identical (earcandy.desktop has the exact same comment!).
So I'm going to comment on the changes you've made versus what's now in the trunk (lp:earcandy revision 50).

I find your approach to the runnable script earcandy interesting. You basically moved the code under if __name__ == "__main__" from the EarCandy.py to the script. Theoretically, this is the best method, practically, I've found from my personal experience with Epidermis that it makes namespaces rather confusing. The other approach which I took calls the script by running subprocess.call(['python', ear_candy.py']), creating two python processes one depending on the other, which is a bit of a waste but makes the result in System Monitor prettier. The final approach (that I can think of) is to launch it by running subprocess.Popen(['earcandy.py']) which is lighter on resources but less pretty in System Monitor.

Since you're using distutils, the data files will be separated from the .py files, so you created a function called find_data_file, similar to my find_program_file. You check to see in which directory of /usr/ or /usr/local/ the files are installed. This actually superfluous, they're always going to be under sys.prefix, according to the distutils documentation.

Having a seperate EarCandyInfo.py file for the application's metadata is certainly interesting. Having one place to modify the program's version number is nice, but do you really need it for things like APPNAME?

I'm going to go ahead and comment on your Ubuntu packaging branch too, lp:~mbudde/earcandy/ubuntu revision 59 if you don't mind. You included a dependency which I somehow forgot: pulseaudio, well done! Having the Ubuntu packaging in a separate branch from the trunk is an interesting idea, and one which a lot of projects go by. I personally prefer having the debian/ folder in the original source, keeps the developers responsible for the packaging and things like dependencies.

Obviously, I'm not the developer of EarCandy, it'll be up to KillerKiwi to decide what to take and what to keep. Thanks for contributing!

« Back to merge proposal