Packaging a single-file Python copy-tool

I'm currently working on a very simple one-file project:

Lumix provides the possibility for the camera TZ41 (and others) to load GPS data and tourist information from a DVD to a SD-card so that you can see on the camera (which has GPS and the SD card inside) this information. Sadly, the software to copy it on the SD card (with the required folder structure) is not available for Linux. So that's what my lumix-maptool does.

As this should be simple to use, I wanted to create a Debian package. But this is not so simple, so I've created a Python package first and I hope a working Python package will make packaging for Debian easier. But it also is the first time I've created a Python package.

I want the user to be able to install the package and then enter lumixmaptool and get the program. This currently works.

Here are some relevant links:

• GitHub project repository

• PyPI page

Currently, my project structure is:

.
├── dist

3 directories, 19 files


setup.py

from setuptools import setup

setup(
name='LumixMaptool',
version='1.0.9',
author='Martin Thoma',
author_email='info@martin-thoma.de',
packages=['lumixmaptool'],
scripts=['lumixmaptool/lumixmaptool.py'],
url='http://pypi.python.org/pypi/LumixMaptool/',
license='LICENSE',
description='Manage GPS information for Panasonic Lumix cameras.',
long_description="""Panasonic offers GPS metadata to add to a SD card. This metadata can contain
tourist information that might be useful for sightseeing. This maptool helps
to copy the data from Lumix DVD to the SD card that is inserted into your
computer (the camera has not to be connected).""",
install_requires=[
"argparse >= 1.2.1",
"pyparsing >= 2.0.1",
"pyparsing >= 2.0.1",
],
entry_points={
'console_scripts':
['lumixmaptool = lumixmaptool:main']
}
)


lumixmaptool.py

#!/usr/bin/env python

"""
Author: Martin Thoma ,
based on https://github.com/Rol

This is really nice code. Clean, documented, idiomatic. I couldn't have written it any better, but that doesn't mean I wouldn't find stuff to improve further ;-)

Regarding the Code

For example, this regex: \s(\d{8})\s(\d{8})\s(.)\s. The \s at either end is completely irrelevant, because your regex isn't anchored anywhere and you do not capture their values. Note furthermore that due to the DOTALL option, the . will consume all remaining characters, so not only is the trailing \s but also the findall unnecessary, use search instead:

match = re.search("(\d{8})\s*(\d{8})\s*(.*)", mapdata)
if not match:
    # handle error because of malformed data
num1, num2, data = match.groups()

Should you actually have wanted to anchor at the beginning, reintroduce the leading \s* but use re.match instead of re.search.

If you really want to precompile the regex, do so outside of your function. Likewise, the grammar could be assembled outside of parse_mapdata. It furthermore irks me that you are reassigning parts of the grammar[1], and could have abstracted over the foo = foo.setResultsName("foo"); foo = foo.setName("foo") e.g. with a function like this:

def named(name, rule):
    return rule.setResultsName(name).setName(name)

regionnum = named("region-number", Word(nums, exact=2))
filename = named("filename", Word(alphanums + "/."))
regiondata = named("region-data",
                   Word("{").suppress() + OneOrMore(filename) + Word("}").suppress())
region = named("region", (regionnum + regiondata))

The next eyesore is probably how you initialized the region_mapping. A dict literal would be a bit cleaner:

region_mapping = {
    1: 'Japan',
    2: 'South Asia, Southeast Asia',
    ...

You have the habit of concatenating strings in ways that are not really appropriate, in the sense that these do not aid readability. For example, the line wrapping in

parser.add_argument('--version', action='version', version='%(prog)s '
                    + __version__)

makes sense from a use-as-much-of-each-line-as-possible viewpoint, but it obscures what __version__ is being concatenated with. While PEP-8 states: “The preferred place to break around a binary operator is after the operator, not before it”, I'd rather align the named arguments:

parser.add_argument('--version',
                    action='version',
                    version='%(prog)s ' + __version__)

New lines are cheap, so we can use lots of them :) I'd suggest the one-named-argument-per-line for the remaining argument parser setup as well, but that's a personal preference.

Related to concatenation, is this:

helptext = "The space-separated indices of the regions to copy. "
helptext += "E.g. 1 6 10. At least one region needs to be given. "
helptext += "Regions are:\n"
tmp = map(lambda i: "%i -\t%s" % (i, region_mapping[i]), range(1, 11))
helptext += "\n".join(list(tmp))

It makes sense where you are breaking up the strings, but I'd rather concatenate instead of append:

helptext =
    "The space-separated indices of the regions to copy. " +\
    "E.g. 1 6 10. At least one region needs to be given. " +\
    "Regions are:\n" +\
    "\n".join(["%i -\t%s" % (i, region_mapping[i]) for i in range(1, 11)])

Note also the for-comprehension instead of map. I see that my suggested solution uses backslashes (which are sometimes unavoidable) and has a line length over 79 characters. As jcfollower points out in an above comment, this could be partially solved with implicit concatenation:

helptext = (
    "The space-separated indices of the regions to copy. "
    "E.g. 1 6 10. At least one region needs to be given. "
    "Regions are:\n"
    ("\n".join(["%i -\t%s" % (i, region_mapping[i]) for i in range(1, 11)]))
)

The extra parens around the join are mandatory in this case, because the implicit concatenation has a higher precedence than a method call (WTF, Python?).

One issue that remains here is the magic number range(1, 11), which should ideally have been taken directly from region_mapping.

In autodetect_mapdata you've copy&pasted a small piece of code:

[f for f in os.listdir(path)
    if os.path.isdir(os.path.join(path, f))]

This would become much more readable with a small helper function:

def autodetect_mapdata():
    """Try to find the DVD with map data."""
    def subdirectories(*path_fragments):
        path = os.path.join(*path_fragments)
        subdirs = [f for f in os.listdir(path) if os.path.isdir(os.path.join(path, f))]
        return path, subdirs

    path, subdirs = subdirectories("/media")
    while len(subdirs) == 1: 
        path, subdirs = subdirectories(path, subdirs[0])

    if "MAP_DATA" in subdirs:
        return os.path.join(path, "MAP_DATA", "MapList.dat")
    return ""

This also happens to get rid of the entertaining

path = path = os.path.join(path, "MAP_DATA/MapList.dat")
return path

Regarding the Pr

StackExchange Code Review Q#46587, answer score: 7