Request for criticism - tell me how to become more Pythonic

[Andrew Dalke]

Jon:
> If anyone has the time, I'd be interested in comments
> about my style: what I am doing right, and what I need
> to improve.

You've a lot of code, and much of it needs a bit of changing.
And I can't give it enough time to think about how to make
the whole thing more Pythonic.  What follows is only
addressing parts.

Turn
        options = {
                'screen_width':         80,
                'description_file':     "info.txt",
                ...

into
        options = {
                'screen_width':         (80, "w"),
                'description_file':     ("info.txt", "f"),
                ...
and move the initialization of 'options' to module
scope (makes it easier to understand what 'process_command_line'
does and reduces typo errors from having to repeat the key
twice).  Could also move short_options and long_options to module
scope.

Much as you like list comprehensions,

short_options = "".join([short_form[opt]+':' for opt in options.keys])

the following is easier for me to understand

short_options = ":".join(options.keys) + ":"

The code all inside the try block
    try:
       (matched_list, remainder) = \
              getopt(sys.argv[1:], short_options, long_options)
        [lots of code]
    except:
       ...

should have much less of the code in the try block, and should
not do a blanket except:

    try:
        matched_list, remainder = getopt(sys.argv[1:] ... )
    except getopt.error, err:
        usage(sys.stderr)
        raise SystemExit(str(err))

(Note that this means having a 'usage' function which takes
an output file handle.  Very useful since it also allows you
to have a --help command line option, which then just calls
usage(sys.stdout).)

def usage(outfile):
    print >>outfile, the_program_title
    print >>outfile, "-"*len(the_program_title)
    print >>outfile,
    print >>outfile, sys.argv[0]," [--options] [information file name]"
    print >>outfile,
    print >>outfile, "Possible command line arguments:"

    keys = options.key()  # Note that I'm sorting the keys
    keys.sort()
    for key in keys:
           default, short_form = options[key]
           print >>outfile, "\t-%s\t--%s" % (short_form, key)


In the main sys.argv parsing loop, I'll often raise a SystemExit
exception rather than
   raise "Too many arguments on command line"

That should not be done if you want to use the parsing code as
part of a larger library.  In that case, your function would
need to take the argv to use (instead of looking up sys.argv[1:])
and would need to declare its own exception class, which would
be explicitly caught in the main().  You're not interested in
that for this code so I won't go into details.

Try replacing your argument processing to

  args = {}
  for k, v in matched_list:
    if k.startswith("--"):
        assert options.has_key(k[2:]), k
        args[k[2:]] = v
    elif k.startswith("-"):
        assert options.has_key(k[1:]), k
        args[k[1:]] = v
    else:
        raise AssertionError(k)


(the assertion checks arise from a general desire to double
check that the input really is parsed correctly.  I've never
had getopt fail on me to produce those exceptions.)

then replace your post-processing to

   for k, (default, short_form) in options.items():
        if args.has_key(k):
            if isinstance(default, type(0)):
                args[k] = int(args[k])
            elif isinstance(default, type("")):  # You don't need these
                pass                             # lines for your code,
            elif isinstance(default, type(0.0)): # since you only have
                args[k] = float(args[k])         # strings and ints.
        else:
           args[k] = default

(In 2.2 you can replace this with

    for k, (default, short_form) in options.items():
        if k in args:
            args[k] = type(default)(args[k])
        else:
            args[k] = default

>    # finally, turn the options into global variables so all the other
>    # functions can use them easily
>              for (option, value) in options.items():
>                     globals()[option] = value

Very non-Pythonic.  Just pass the list of arguments around.
Better yet, I have my main code know which functions use which
arguments.

You use string exceptions:
>        if len(input)<len(header_fields):
>                raise "Input error: Album description truncated

These are deprecated.  If you don't want to make your own
exception type, then for these sorts of errors I like using
AssertionError, as in

    assert len(input) >= len(header_files), "Input error: Alb ..."

The variable name 'input' in things like
> def parse_album_information(input):

isn't descriptive enough.  It looks like a list of lines, but
other implementations could use, say, a file handle.  If you
say 'input_lines' then it's easier to understand.

You could also recast your input code as a reader:

   for disk_info in DiskInfoReader(open(description_file, 'r')):
      tracklist_info = parse_track_list(disk_info)
      display_tracklist(disk_info, tracklist_info)

# Switch to using an iterator for 2.2
class DiskInfoReader:
    def __init__(self, file):
        self.file = file
        self._n = 0
    def __getitem__(self, i):
        assert self._n == i, "forward iteration only"
        ...
        self._n += 1
        return disk_info

but I don't understand your code well enough to do the
refactoring for that, or even if it's appropriate (there
seem to be two different chunks of code that alternate
consuming from input_lines.)

Don't use 'except:', as in
>        try:
>                os.makedirs(destination)
>        except:

Check for the minimal error you can, which is os.error in
this case.

build_command_line should ensure the info_value passed to
the 'encoder_option_format[info_name]' string doesn't contain
a " or other escape character.  I always find escapeing
characters for the shell an annoyance.  I've only found one
function in the std. Python libs that helps - commands.mkarg,
and it always adds a space at the front.

Uggh, I knew this would take a long time to write.  That's
why few others have responded to this thread.

                    Andrew
                    dalke at dalkescientific.com