Packaging a single-file Python copy-tool

Question

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 <- automatically generated
│   ├── LumixMaptool-1.0.5.tar.gz
│   ├── LumixMaptool-1.0.6.tar.gz
│   ├── LumixMaptool-1.0.7.tar.gz
│   ├── LumixMaptool-1.0.8.tar.gz
│   └── LumixMaptool-1.0.9.tar.gz
├── LICENSE.txt
├── lumixmaptool
│   ├── __init__.py
│   └── lumixmaptool.py
├── lumix-maptool.desktop  <- needed for Debian packaging
├── LumixMaptool.egg-info <- automatically generated
│   ├── dependency_links.txt
│   ├── entry_points.txt
│   ├── PKG-INFO
│   ├── requires.txt
│   ├── SOURCES.txt
│   └── top_level.txt
├── lumix-maptool-icon.png  <- will be used for Debian packaging
├── README.md
├── setup.cfg
└── setup.py

3 directories, 19 files

setup.py

from setuptools import setup

setup(
    name='LumixMaptool',
    version='1.0.9',
    author='Martin Thoma',
    author_email='[email protected]',
    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 <[email protected]>,
        based on https://github.com/RolandKluge/de.rolandkluge.lumix_map_tool/
        from Roland Kluge.

Manage GPS information for Panasonic Lumix cameras.

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).
This script was tested with Lumix TZ41.
"""

import os
import re
import shutil
import logging
from argparse import ArgumentParser, RawTextHelpFormatter, Action
from pyparsing import Word, nums, OneOrMore, alphanums

logfile = os.path.join(os.path.expanduser("~"), 'maptool.log')
logging.basicConfig(filename=logfile, level=logging.INFO,
                    format='%(asctime)s %(message)s')

__version__ = "1.0.9"

region_mapping = {}
region_mapping[1] = 'Japan'
region_mapping[2] = 'South Asia, Southeast Asia'
region_mapping[3] = 'Oceania'
region_mapping[4] = 'North America, Central America'
region_mapping[5] = 'South America'
region_mapping[6] = 'Northern Europe'
region_mapping[7] = 'Eastern Europe'
region_mapping[8] = 'Western Europe'
region_mapping[9] = 'West Asia, Africa'
region_mapping[10] = 'Russia, North Asia'


def is_valid_mapdata(parser, path_to_mapdata):
    """Check if path_to_mapdata is a valid path."""
    if os.path.isfile(path_to_mapdata):
        return path_to_mapdata
    else:
        if path_to_mapdata == '':
            parser.error("You have to specify the path to the mapdata file "
                         + "(it's on a DVD).")
        else:
            parser.error("The file '%s' does not exist." % path_to_mapdata)


def is_valid_sdcard(parser, path_to_sdcard):
    """Check if sdcard is a valid path."""
    if not os.path.exists(path_to_sdcard):
        parser.error("The path '%s' does not exist." % path_to_sdcard)
    if not os.access(path_to_sdcard, os.W_OK):
        parser.error("The path '%s' is not writable" % path_to_sdcard)
    else:
        return path_to_sdcard


def parse_mapdata(path_to_mapdata):
    with open(path_to_mapdata, 'r') as f:
        mapdata = f.read()
    mapdata_pattern = re.compile("\s*(\d{8})\s*(\d{8})\s*(.*)\s*", re.DOTALL)
    num1, num2, data = mapdata_pattern.findall(mapdata)[0]

    parsed_map_data = {'num1': num1, 'num2': num2, 'regions': {}}

    def regionParsing(x):
        parsed_map_data['regions'][int(x[0])] = x[1:]

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

    data = data.strip()  # a strange character at the end
    map_grammar.parseString(data)

    return parsed_map_data


def copy_maps(path_to_mapdata, path_to_sdcard, regions):
    """Copy map information of regions to sdcard."""
    mapdata_cd_folder = '/'.join(path_to_mapdata.split("/")[:-1])
    logging.info("mapdata_cd_folder: %s" % mapdata_cd_folder)
    #mapdata_on_sdcard = path_to_sdcard + "/PRIVATE/MAP_DATA"

    # This works with Panasonic Lumix DMC TZ-41
    mapdata_on_sdcard = os.path.join(path_to_sdcard,
                                     "PRIVATE/PANA_GRP/PAVC/LUMIX/MAP_DATA")
    if not os.path.exists(mapdata_on_sdcard):
        os.makedirs(mapdata_on_sdcard)
    mapdata = parse_mapdata(path_to_mapdata)

    # Delete previously stored cards
    shutil.rmtree(mapdata_on_sdcard, ignore_errors=True)

    # And create the directory again
    os.makedirs(mapdata_on_sdcard)

    for selected_region_id in regions:
        print("Copying region '%s' ..." % selected_region_id)
        for path in mapdata['regions'][selected_region_id]:
            logging.info("Copy file %s" % path)
            subdir, filename = path.split("/")
            abspath_to_source_file = os.path.join(mapdata_cd_folder, path)
            target_dir = mapdata_on_sdcard + "/" + subdir
            target_file = target_dir + "/" + filename

            logging.info("abspath_to_source_file: %s" % abspath_to_source_file)
            logging.info("target_dir: %s" % target_dir)
            logging.info("target_file: %s" % target_file)

            if not os.path.exists(target_dir):
                os.mkdir(target_dir)

            if not os.path.exists(target_file):
                shutil.copy(abspath_to_source_file, target_dir)
        print("Copying region '%i' DONE" % selected_region_id)
    print("All operations exited succesfully.")


class UniqueAppendAction(Action):
    """Make sure that the list of regions contains unique values."""
    def __call__(self, parser, namespace, values, option_string=None):
        unique_values = set(values)
        setattr(namespace, self.dest, unique_values)


def autodetect_mapdata():
    """Try to find the DVD with map data."""
    path = "/media"
    subdir = [f for f in os.listdir(path)
              if os.path.isdir(os.path.join(path, f))]
    while len(subdir) == 1:
        path = os.path.join(path, subdir[0])
        subdir = [f for f in os.listdir(path)
                  if os.path.isdir(os.path.join(path, f))]

    if "MAP_DATA" in subdir:
        path = path = os.path.join(path, "MAP_DATA/MapList.dat")
        return path
    return ""


def main():
    parser = ArgumentParser(description=__doc__,
                            formatter_class=RawTextHelpFormatter)

    # Add more options if you like
    parser.add_argument("-m", "--mapdata", dest="mapdata", metavar="MAPDATA",
                        default=autodetect_mapdata(),
                        help="path to MAPDATA folder on the Lumix DVD",
                        type=lambda x: is_valid_mapdata(parser, x))
    parser.add_argument("-s", "--sdcard", dest="path_to_sdcard",
                        metavar="SDCARD", required=True,
                        help="path to SDCARD",
                        type=lambda x: is_valid_sdcard(parser, x))

    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))
    parser.add_argument("-r", "--regions", dest="regions", nargs='+',
                        required=True, choices=list(range(1, 11)), type=int,
                        action=UniqueAppendAction,
                        help=helptext)
    parser.add_argument('--version', action='version', version='%(prog)s '
                        + __version__)
    args = parser.parse_args()
    copy_maps(args.mapdata, args.path_to_sdcard, args.regions)

if __name__ == "__main__":
    main()

init.py

#!/usr/bin/env python
import lumixmaptool

if __name__ == "__main__":
    lumixmaptool.main()

Especially I would like to know if the way I currently use the __init__.py is correct and if my project structure makes sense. But I'm also interested in general coding advice.

I've used http://pep8online.com/ to check for "obvious" advice.

Answer 1

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 Project Structure

This is done quite nicely, although __init__.py is not used correctly. When a user says import lumixmaptool, this file will be executed – that is, the sole purpose of this file is to allow a directory to be used as a package. The symbols in this file's namespace make up the public API. If I understand Python packaging correctly, users would currently have to say from lumixmaptools import lumixmaptools to get actual access to the functions.

Instead, lumixmaptool/__init__.py should probably be:

# this will import symbols from the inner, internal package
from . lumixmaptools import __version__, main, copy_maps

It would make sense to rename the internal package to avoid confusion.

The if __name__ == "__main__" trick does not work here, unless you invoke the package by a full path: python /wherever/its/installed/lumixmaptool/__init__.py. If you want to execute it as a command line tool via python -m lumixmaptool, we offer a __main__ submodule (file lumixmaptool/__main__.py):

from . __init__ import *

main()

We can furthermore improve the setup.py.

• license should rather be an actual license like MIT.
• don't list your module in scripts, now that we can invoke it properly
• you could also specify classifiers and keywords for your module.

I noticed that your project doesn't have any tests. This is understandable, but unacceptable if you want to distribute this. At the very least, a check that the module compiles successfully would be a step forward. If you change parse_mapdata to take not the filename but the contents of the file as argument, it will be very straightforward to add a few tests for this crucial function as well.

---

[1]: I am a fan of single-assignment form

Answer 2

On top of amon's comment about region_mapping :

A dict literal would be a bit cleaner.

It would indeed be better than the way you are setting the dict at the moment. However, an even simpler solution could be done : just use a list and generate the indices on demand with enumerate() :

region_mapping = ['Japan', 'South Asia, Southeast Asia', 'Oceania', 'North America, Central America', 'South America', 'Northern Europe', 'Eastern Europe', 'Western Europe', 'West Asia, Africa', 'Russia, North Asia']
print "\n".join("%i -\t%s" % (i+1,r) for i,r in enumerate(region_mapping))

Many advantages :

• you don't have to care about the indices.
• you don't have to care about the length of the dictionnary.