Renaming PDF files based on given rules in a text file

Question

I'm learing Python by writing a program to solve some housekeeping I do weekly, namely renaming PDF files according to rules given in a text file.

My end goal is to have idiomatic Python, with full test coverage. I have done some refactoring, and written a couple of tests. Ideally I would have done TDD from the beginning, but I find it challenging to write tests since I don't know Python that well yet.

The PDF documents can be named with two patterns:

xx.pdf
xx-y.pdf

They are stored in the folder ./test relative to the script root.

The text file Instruments.txt contains the following pattern:

01 SopranoCn
02 SoloCn
03 RepianoCn
...
18 Percussion
20 AllParts
...

The whole purpose is to rename files like this:

xx.pdf   (01.pdf)   --> xx_{instrument_name}.pdf (01_SopranoCn.pdf)
xx-y.pdf (02-1.pdf) --> xx_{instrument_name}{part_number}.pdf (02_SoloCn1.pdf)

The y-argument is optional, and can be added to any instrument.

Here's the whole source:

"""
Invoke with "python notesort.py <path to folder>".
A list of instrument numbers with corresponding instrument names
must be available in the root folder of the script.
"""

# Standard library imports
import os

FOLDER = './test'


def create_instrument_map():
    """Create a dictionary mapping each prefix
    to the corresponding instrument name."""
    with open('Instruments.txt') as instrument_file:
        return {
            line.split()[0]: line.split()[1] for line in instrument_file
        }


def rename_files_with_instrument_name(instrument_map):
    """Rename PDF files in folder based on the root of
    the original file name."""
    for filename in os.listdir(FOLDER):
        root, extension = os.path.splitext(filename)
        if not extension.endswith('.pdf'):
            continue
        try:
            if root[2]:
                if root[:2] in instrument_map:
                    new_file_name = '{prefix}_'\
                                    '{instrument_name}'\
                                    '{part_number}'\
                                    '{extension}'.format(
                                        prefix=root[:2],
                                        instrument_name=instrument_map[root[:2]],
                                        part_number=root[3],
                                        extension=extension)
        except:
            if root in instrument_map:
                new_file_name = '{prefix}_{instrument_name}{extension}'.format(
                                    prefix=root,
                                    instrument_name=instrument_map[root],
                                    extension=extension)
        old_path = os.path.join(FOLDER, filename)
        new_path = os.path.join(FOLDER, new_file_name)
        os.rename(old_path, new_path)
        print 'Renamed', filename, '-->', new_file_name

if __name__ == '__main__':
    instrument_map = create_instrument_map()
    rename_files_with_instrument_name(instrument_map)

I have started to write some tests for it, but I find it difficult to test rename_files_with_instrument_name. I do not have any experience writing tests, but I see the benefit and want to practise writing them.

These are the tests I have so far:

import unittest
from notesort import create_instrument_map


class TestNoteSort(unittest.TestCase):

    def test_create_instrument_map_creates_soprano_cornet(self):
        instrument_map = create_instrument_map()
        self.assertEqual(instrument_map['01'], 'SopranoCn')

    def test_create_instrument_map_soprano_not_solo(self):
        instrument_map = create_instrument_map()
        self.assertNotEqual(instrument_map['01'], 'SoloCn')

if __name__ == '__main__':
    unittest.main()

Please feel free to make comments on any part of my implementation. No need to be easy on me, even if I'm a beginner.

I am especially interested in how I can get further with testing, and also how to refactor rename_files_with_instrument_name.

Answer 1

Avoid hard-coding the names of files and directories. Use parameters instead (with default values, if you prefer).

The rename_files_with_instrument_name() function seems complicated and fragile. It's complicated because it tries to do two things: figure out what the new filename should be, and perform the renaming. Therefore, it would be better to split them into two functions. It's fragile in that you are blindly slicing strings at certain indexes, checking only that the file extension is .pdf. If you ever define prefixes that are not two characters long, or if you run the same script twice on a directory, the results could be nasty and somewhat difficult to undo. More careful validation of the filename would be good. Alternatively, use a regular expression substitution so that you only rename files that fit the pattern.

import os
import re

def instrument_map(map_filename='Instruments.txt'):
    with open(map_filename) as f:
        return dict(line.rstrip().split(' ', 1) for line in f)

def filename_for(instrument_map, filename):
    return re.sub(r'^(?P<prefix>[^.-]+)(?:-(?P<part>.*))?\.pdf$',
        lambda match: '{prefix}_{instrument}{part}.pdf'.format(
            prefix=match.group('prefix'),
            instrument=instrument_map[match.group('prefix')],
            part=match.group('part') or '',
        ) if match.group('prefix') in instrument_map else match.string,
        filename
    )

def rename_files(filename_map, folder='./test'):
    for old_filename in os.listdir(folder):
        new_filename = filename_for(filename_map, old_filename)
        if new_filename != old_filename:
            os.rename(os.path.join(folder, old_filename),
                      os.path.join(folder, new_filename))
            print 'Renamed {0} --> {1}'.format(old_filename, new_filename)

if __name__ == '__main__':
    rename_files(instrument_map())

Answer 2

For now I'll just throw up a few random thoughts.

The first thing I would do with rename_files_with_instrument_name is get rid of its reliance on the global FOLDER variable. Take a directory path as an argument, or shove it in a class and make the folder a parameter to the class's __init__ method which gets stored in an instance variable. That should immediately make it easier to write tests for rename_files_with_instrument_name; just pass in a folder and check that you get the results you expect.

The second thing I would do with rename_files_with_instrument_name is try and shorten the name a little. It's very long. Something like add_instrument can be good enough in context, but even rename_with_instrument is an improvement.

Going inside rename_files_with_instrument_name, using bald except is usually a bad practice. I gave an example case where bald except is bad in my answer to another question. If you think an exception might be thrown, it's better to name the specific exception. Also, make sure the situation where the exception gets raised is actually exceptional (an error or some situation which some code outside the function should handle). It looks a little like you're handling a normal, expected case in your except block—the case where the root filename is shorter than three characters. It's better to handle those with if. Not only is it easier to understand, it's also actually faster; catching exceptions is fairly time-consuming in most languages.

However, you don't actually need a conditional here; if you try to take a slice of a string which is longer than the entire string, the entire string just gets returned. That is:

>>> s = "ab"
>>> s[:30]
"ab"

So you can just write, without the try block, if root[:2] in instrument_map.

It's also usually good to avoid continue if you can. Sometimes you can't, and in those cases, don't contort yourself. But I think you can here. It looks like you could just write:

if extension.endswith(".pdf"):
    # All the code to construct a new filename