Batch rename flac files

This is my first real program in python (and my first real program) and I would like to have input by some more advanced programmers on the code, on the writing style and on the amount of comments (is it clear enough?).

new_name(scheme) does seems a bit bloated to me, but I can't figure out how to write it in a simpler way.

The program is made to run on Debian GNU/Linux exclusively (if I get a good feedback I'll port it to other distros)

```
#!/usr/bin/python
# -- coding: utf-8 --

"""
rename-flac takes the information from FLAC metadata to batch rename
the files according to a filenaming scheme.

Usage:
rename-flac.py
rename-flac.py -o
rename-flac.py (-h | --help)
rename-flac.py --version

Arguments:
The filenaming scheme. Has to be between quotation marks
The path to the directory containing the album

Options:
-o Displays filenaming scheme options
-h --help Shows the help screen
--version Outputs version information

"""
from docopt import docopt # Creating command-line interface

import sys
import subprocess

from py.path import local

#TODO: write to work with Python 3

# Dependency check
programlist = ["flac", "python-py", "python-docopt"]
for program in programlist:
pipe = subprocess.Popen(
["dpkg", "-l", program], stdout=subprocess.PIPE)
dependency, error = pipe.communicate()
if pipe.returncode:
print """
%s is not installed: this program won't run correctly.
To instal %s, run: aptitude install %s
""" % (program, program, program)
sys.exit()
else:
pass

# Defining the function that fetches metadata and formats it
def metadata(filename):
filename = str(filename).decode("utf-8") # Since filename is a LocalPath

# Tracknumber
pipe = subprocess.Popen(
["metaflac", "--show-tag=tracknumber", filename],
stdout=subprocess.PIPE)
tracknumber, error = pi

1. Comments on your code

-
I like the way you've used docopt. But why do you require the user to run rename-flac.py -o in order to read the documentation for the schemes? Surely it would be clearer and simplier if the documentation for the schemes were included in the usage documentation?

-
Your program would be easier to test if you put the top-level code into a function and then guarded the function call with if __name__ == '__main__'. At the moment if you try to import the script from the interactive interpreter, the script runs.

-
Your program would be easier to test if it didn't call sys.exit(), but instead just finished running. At the moment, when testing it from the interactive interpreter, whenever something goes wrong then it quits the interpreter, which is annoying.

-
You use py.path but this seems like overkill to me given that you don't use it for anything complicated. Instead you could use the built-in os module and avoid the extra dependency on py.path. (Use os.walk instead of LocalPath.visit and os.rename instead of LocalPath.rename.)

-
Your function rename takes an argument root but this is not used. The function happens to work, but that's just by luck, because filename is a global variable: but if you move the top-level code to a function, then it will break.

In fact, you make a lot of use of global variables. It's better to pass all the variables you need as function parameters: that way, you can be sure that you've got the right variable, and not some other variable that happens to have the same name.

-
You write:

#TODO: write to work with Python 3

but it's not too hard to make your code portable between Python 2 and Python 3. You have to change your print statements:

print "Files renamed"

so that they have parentheses:

print("Files renamed")

and you have to decode the output from metaflac.

-
Your "dependency check" runs dpkg to see if certain packages are installed. But that's not very portable. How do you know that the user used dpkg to install the Python packages? Maybe they used pip or easy-install instead? How do you know that they are on an operating system that uses dpkg at all?

I would avoid this dependency check altogether. For the docopt package it's too late to check since you've already executed from docopt import ... at this point. Someone who hasn't installed docopt will get:

ImportError: No module named 'docopt'

And if metaflac is not installed, then when you try to run it you'll get:

FileNotFoundError: [Errno 2] No such file or directory: 'metaflac'

-
You write:

pipe = subprocess.Popen(
    ["dpkg", "-l", program], stdout=subprocess.PIPE)
dependency, error = pipe.communicate()
if pipe.returncode:

but you do not use dependency or error. If you just want to throw away the output, you can replace the above with:

if subprocess.call(["dpkg", "-l", program], stdout=subprocess.DEVNULL):

-
Your function rename starts by checking:

if "t" not in permanent_scheme:  # To have different file names
    print(" %t has to be present in ")
    sys.exit()

but wouldn't it be better to do this check just once, rather than for each file? Also, shouldn't the test be "%t" not in permanent_scheme?

-
Error messages from a command-line program ought to go to standard error, not standard output, so this:

print "%t has to be present in "

should be:

sys.stderr.write("%t has to be present in \n")

-
It's not actually clear to me why %t has to be present in the scheme. Obviously something has to be there to ensure that the filenames are different, but surely %n would do just as well?

-
You write:

scheme = args[""]

but this variable never gets used: it always gets overwritten by:

scheme, formatter = new_name(permanent_scheme)

-
The function new_name does two things. First, it rewrites the scheme replacing %t with %(title)s and so on. This is always the same. Second, it creates a dictionary formatter with keys title, artist and so on. This is different for each file. It would be better if you split up these functions: have one function that rewrites the scheme (which you could call just once), and another that creates the dictionary (which you would call for each file).

In fact, why not change the function metadata so that it creates and returns the dictionary?

-
The output on success is just:

Files renamed

but I think it would be more useful to output a description of the renames that took place.

-
In metadata it would be better to write a loop than to copy out the code for running metaflac six times. This loop should be driven by a data structure that lists the tags you are going to look up.

-
Similarly, in new_name it would be better to write a loop than to copy out the code for replacing a string six times. Or, by using re.sub, you could do in one function c

StackExchange Code Review Q#37721, answer score: 12