Cross-platform command line tool for finding files

Question

#!/usr/bin/env python

''' Search for files using wildcards amd magic '''

import os
from fnmatch import fnmatch
import sys
import argparse
import magic


def cmd_args():
    ''' returns a argparse namespane '''
    parser = argparse.ArgumentParser()

    parser.add_argument(
        '--root', default='.',
        help='Starting point [ defaults to cwd ]')

    parser.add_argument(
        '--pattern', default='*.*',
        help='Pattern to use for matching file names [ Defaults to *.* ]')

    parser.add_argument(
        '--depth',
        default=2**32,
        type=int,
         help='''
        Depth is the number of slashes in the files path ''')

    parser.add_argument(
        '--links',
        default=True,
        type=bool,
        help='\nFollow links\n')

    parser.add_argument(
        '--magic',
        default=None,
        type=str,
        help='Filter file by a magic keyword link: image data png')

    parser.add_argument(
        '--cat',
        default=False,
        type=bool,
        help='Concatenate matched files')

    return  parser.parse_args()

CMD_ARGS = cmd_args()

def cat(filename):
    ''' cat(filename) -> file contents'''
    if CMD_ARGS.cat:
        try:
            with open(filename, 'r') as file_object:
                print file_object.read()
        except IOError:
            pass
    else:
        return filename


def depth_check(link):
    '''
    This is called on each files path to control depth cutoff.
    It works by counting the number of slashes in the links path.
    If DEPTH > MAX DEPTH {
        EXIT PROGRAM
    }
    Note: By default max depth is 2**32 aka big ass number
    '''

    if link.count('/') > CMD_ARGS.depth:
        sys.exit()
    else:
        return link


def aggragate_files(root, dirs, files):

    _files = [depth_check(os.path.join(root, filename))
              for filename in files if fnmatch(filename, CMD_ARGS.pattern)]

    _subfiles = [depth_check(os.path.join(root, filename))
                 for filename in dirs if fnmatch(filename, CMD_ARGS.pattern)]

    return set(_files + _subfiles)


def find():
    for root, dirs, files in os.walk(CMD_ARGS.root,
                                     followlinks=CMD_ARGS.links):
        handle_files(aggragate_files(root, dirs, files))


def handle_files(files):
    for filename in files:
        if CMD_ARGS.magic:
            meta = magic.from_file(filename)
            if meta and CMD_ARGS.magic.lower() in meta.lower():
                print cat(filename)
        else:
            print cat(filename)


if __name__ == '__main__':
    find()

My concerns with the the code above:

• Is the code easy to read?
• Does this code honor PEP8?
• Have I made any mistakes in the control-flow?
• Would this code be considered pythonic?
• Are my naming conventions descriptive?

How can I structure this code to make it easy to add features?

For example, where and how should this be hooked in?

def cat(filename):
        ''' cat(filename) -> file contents'''
        if CMD_ARGS.cat:
            try:
                with open(filename, 'r') as file_object:
                    print file_object.read()
            except IOError:
                pass
        else:
            return filename

Answer 1

Let's first dispense with PEP8: by and large, as far as I can tell, your code respects PEP8. On the other hand, I've been writing Python professionally for almost 20 years and have never read PEP8 before today.

This code does a number of things that I wouldn't do, however. First, it uses

if __name__ == "__main__"

when there's no way for this code to be run any other way; it parses its command arguments from the arguments of the invocation of Python. This idiom is used to distinguish script behavior from module behavior, and your script behavior is all over the file. If you want handle_files to be useable as an imported function, you'll want to pass it all its arguments, and you'll want the option of not processing command line arguments. So really, all your command-line argument processing should be inside the conditional instead of outside - that is, assuming you want to be able to import this module as well as run it as a script. If you don't care about importing it, don't use the if __name__ == "__main__" idiom.

If you're interested in importing as an option, you also won't want to call sys.exit() in any of your module-level code (e.g., the depth_check function). Raise an error instead, and when you're running the module instead of importing, catch and exit.

I don't see the reason for the cmd_args() function. It's called once, and it creates module-level information; why should it be in a function?

On the other hand, I don't like the references to the globals all over the code - a special no-no if you're worrying about using this module by importing it. If you intend to import, my preferred organization would be:

class HandleFileError(Exception):
    pass

def depth_check(link, depth = ...):
    ... raise HandleFileError ...

def handle_files(files, cat = False, ....):
    ...

if __name__ == "__main__":
    ... process cmd args ...
    try:
        find(options.root, cat = options.cat, ...)
    except HandleFileError:
        ... report the error...
        sys.exit(1)

And you've got enough options that I'd probably create a Finder class and initialize it with the parameters, to hold the state.

On the other hand, if you just want this to be a script, I don't see the point of the find() function - it's called exactly once, after the command line arguments are parsed. I'd get rid of it.

Have you tested this code? As far as I can tell from reading it, the cat option will cause handle_files to print out None for each file that's concatenated. It's an odd mixture, either printing the contents of the files or the filenames; but one way or another, either the cat function should print the filename, or it should return the file contents, but not print in one case and return in another.

A couple small inefficiencies. First, you call CMD_ARGS.magic.lower() every time you check a file. You should call it once, when the finder is initializing, and never again. Second, in aggragate_files, you create two lists, then create a third list by concatenating the two lists, and then turn the third list into a set. At the very least, you can do this:

s = set(_files)
s.update(_subfiles)

But more globally, combining these two elements is wrong in general, because cat will always fail on the directory. Have aggragate_files return a pair, and only call cat on the files; for the dirs, just print them.

Finally, aggragate_files is misspelled; it's aggregate.