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.

• jcfollower: A quick note ... for multiline strings, you can use parenthesis rather than += ... helptext = ('abc' [newline] 'def' [newline] 'ghi')

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

• amon: (and by the way, greetings from Karlsruhe to Karlsruhe)
• Martin Thoma: You're from Karlsruhe? Do you also study computer science at KIT? Do we eventually know each other?
• Martin Thoma: Thanks for your advice. Could you please tell me if the way I use __init__.py is correct and if my project structure makes sense?
• amon: @moose I added an update regarding the project structure, but keep in mind that I'm not terribly knowledgeable about this kind of stuff. I do study at the KIT, but CS is only my minor. I recognized you from the Facebook group, but we don't know each other IRL.

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.