Monitor filesystem for continuous integration and build

Question

I've written a simple Python module that depends on watchdog to monitor for modified files, then runs various integration and build processes.

I'm fairly new to Python, so I'd appreciate all criticism. For example of how I'm using the module see this.

import sys, os, time, copy, logging
from watchdog.observers import Observer
from watchdog.events import FileSystemEventHandler


class ChangeHandler(FileSystemEventHandler):
    def __init__(self, callback):
        if callable(callback):
            self.callback = callback
        else:
            raise TypeError('callback is required')
    def on_modified(self, event):
        if event.is_directory or event.event_type is not "modified":
            return
        self.callback(event.src_path)

class Watcher(object):
    file_set = {}
    def _trim_root(self, path, root = None):
        if root is None:
            root = self.root
        if path.startswith(root):
            path = path[len(root):]
        if path.startswith('/'):
            path = path[1:]
        return path
    def _on_file_changed(self, src_path):
        src_path = self._trim_root(src_path)
        for out, props in self.file_set.iteritems():
            files = props['files']
            callbacks = props['callbacks']
            if src_path in files:
                if callable(callbacks['onchange']):
                    if not callbacks['onchange'](os.path.abspath(src_path)):
                        raise Exception('onchange callback errored for file ' + src_path)
                print os.path.abspath(out)
                self._compile(files, callbacks, os.path.abspath(out))

    def _compile(self, files, callbacks, out):
        if hasattr(callbacks, 'mode') and callbacks['mode'] is 'slurp':
            slurpy = [out, []]
            for fname in files:
                with open(fname) as infile:
                    slurpy[1].append([fname, infile.read()])
            if hasattr(callbacks, 'each') and callable(callbacks['each']):
                callbacks['each'](slurpy)
            else:
                raise Exception('slurp mode requires an each handler')
        else:
            # default mode is concat, also contains break for files_only mode
            if hasattr(callbacks, 'each') and callable(callbacks['each']):
                rv = callbacks['each']([out, files])
                if type(rv) is list:
                    files = rv
                elif rv is False:
                    logging.info('each callback asked us to exit quietly')
                    return
                elif rv is True:
                    logging.info('finished each callback, proceeding with original file list')
                else:
                    raise TypeError('the each callback must return an array of file names or True')
            if hasattr(callbacks, 'mode') and callbacks['mode'] is 'files_only':
                logging.info("files_only mode, no concat or post")
                return
            with open(out, 'w') as outfile:
                for fname in files:
                    with open(fname) as infile:
                        for line in infile:
                            outfile.write(line if not hasattr(callbacks, 'line') else callbacks['line'](line))
            logging.info("Wrote file: " + out)
            if hasattr(callbacks, 'post') and callable(callbacks['post']):
                callbacks['post'](out)

    def compile(self):
        for out, props in self.file_set.iteritems():
            files = props['files']
            callbacks = props['callbacks']
            self._compile(files, callbacks, os.path.abspath(out))

    def FileSet(self, out = None, files = None, callbacks = {}):
        if type(files) is not list:
            raise TypeError('files list is a required  argument')
        if type(out) is not str:
            raise TypeError('out is a required argument')
        if hasattr(self.file_set, out):
            raise Exception('out file is already in use by another FileSet')
        if out in files:
            raise Exception('cannot watch out file')
        self.file_set[out] = {'files' : files, 'callbacks' : callbacks}
        return out

    def MirrorSet(self, orig = None, new = None, addl = None):
        if type(orig) is not str:
            raise TypeError('orig list is a required  argument')
        if type(new) is not str:
            raise TypeError('new is a required argument')
        try:
            files = copy.deepcopy(self.file_set[orig]['files'])
            skin_path = os.path.join(self.root, new)
            out = os.path.join(skin_path, orig)
            for i, filename in enumerate(files):
                filePath = os.path.join(skin_path, filename)
                if (os.path.exists(filePath)):
                    files[i] = os.path.join(new, files[i])
            if addl is not None and type(addl) is list:
                files.extend(addl)
            self.file_set[out] = {'files' : files, 'callbacks' : self.file_set[orig]['callbacks']}
        except:
            raise

    def stop(self):
        self.observer.stop()

    def start(self):
        event_handler = ChangeHandler(self._on_file_changed)
        self.observer = Observer()
        self.observer.schedule(event_handler, self.root, recursive=True)
        self.observer.start()

    def __init__(self, root = None):
        if type(root) is not str:
            raise TypeError('root is required as argument in object instantation')
        self.root = os.path.abspath(root)

Answer 1

import sys, os, time, copy, logging
from watchdog.observers import Observer
from watchdog.events import FileSystemEventHandler


class ChangeHandler(FileSystemEventHandler):
    def __init__(self, callback):
        if callable(callback):
            self.callback = callback
        else:
            raise TypeError('callback is required')

    def on_modified(self, event):
        if event.is_directory or event.event_type is not "modified":

Don't use is to compare strings, you've got no guarantees it'll do anything useful. Use == return self.callback(event.src_path)

I think this would be clearer as:

 if not event.is_directory and event.event_type == modified:
     self.callback(event.src_path)

As it stands, the logic is obscured.

class Watcher(object):
    file_set = {}

No! This is a class attribute, you almost certainly wanted an object attribute. You should store this __init__ as self.file_set.

    def _trim_root(self, path, root = None):
        if root is None:
            root = self.root

You only ever pass None, so don't have it as a parameter.

        if path.startswith(root):
            path = path[len(root):]
        if path.startswith('/'):
            path = path[1:]
        return path

Look at os.path.relpath, python already does the logic you've got here.

    def _on_file_changed(self, src_path):
        src_path = self._trim_root(src_path)
        for out, props in self.file_set.iteritems():
            files = props['files']
            callbacks = props['callbacks']

Don't use dictionaries as general purpose storage devices. Instead, use objects.

            if src_path in files:
                if callable(callbacks['onchange']):

You just the ignore the callable if its not callable. That's asking for hard to diagnose bugs.

                    if not callbacks['onchange'](os.path.abspath(src_path)):
                        raise Exception('onchange callback errored for file ' + src_path)

Have your callback reports problems with exceptions, not return values.

                print os.path.abspath(out)
                self._compile(files, callbacks, os.path.abspath(out))

    def _compile(self, files, callbacks, out):
        if hasattr(callbacks, 'mode') and callbacks['mode'] is 'slurp':

hasattr checks for attributes, 'mode' is a key. You've got a significant deficiency in your understanding of python there. hasattr will always return false. This suggests to me that you can't possible have done any serious testing of this code.

            slurpy = [out, []]

Lists are for lists of things. That isn't a list of things, that's two things. It should probably be two variables.

            for fname in files:

I recommend spelling out filename, rather then abbreviating it

                with open(fname) as infile:
                    slurpy[1].append([fname, infile.read()])
            if hasattr(callbacks, 'each') and callable(callbacks['each']):

Again, this doesn't work. AT ALL.

                callbacks['each'](slurpy)
            else:
                raise Exception('slurp mode requires an each handler')
        else:
            # default mode is concat, also contains break for files_only mode
            if hasattr(callbacks, 'each') and callable(callbacks['each']):
                rv = callbacks['each']([out, files])

Firstly, its wrong. But secondly, you should have gotten tired of typing this by now and made a function out of it.

                if type(rv) is list:

Generally a better idea to check for types using isinstance, but even better to avoid checking types at all.

                    files = rv
                elif rv is False:

Check for false using not rv

                    logging.info('each callback asked us to exit quietly')
                    return

Throw exceptions, Python loves exceptions

                elif rv is True:

Use elif rv:

                    logging.info('finished each callback, proceeding with original file list')

I suggest returning None and checking for that here

                else:
                    raise TypeError('the each callback must return an array of file names or True')

In python, they are lists not arrays

            if hasattr(callbacks, 'mode') and callbacks['mode'] is 'files_only':
                logging.info("files_only mode, no concat or post")
                return
            with open(out, 'w') as outfile:
                for fname in files:
                    with open(fname) as infile:
                        for line in infile:
                            outfile.write(line if not hasattr(callbacks, 'line') else callbacks['line'](line))
            logging.info("Wrote file: " + out)
            if hasattr(callbacks, 'post') and callable(callbacks['post']):
                callbacks['post'](out)

    def compile(self):
        for out, props in self.file_set.iteritems():
            files = props['files']
            callbacks = props['callbacks']
            self._compile(files, callbacks, os.path.abspath(out))

    def FileSet(self, out = None, files = None, callbacks = {}):

Python cnvention resevers CamelCase for class names. Also, this should be add_fileset

        if type(files) is not list:
            raise TypeError('files list is a required argument')

If the parameter is required, why did you provide a default value!

        if type(out) is not str:
            raise TypeError('out is a required argument')
        if hasattr(self.file_set, out):
            raise Exception('out file is already in use by another FileSet')

Again hasattr is incorrect here, you want out in self.file_set

        if out in files:
            raise Exception('cannot watch out file')
        self.file_set[out] = {'files' : files, 'callbacks' : callbacks}
        return out

    def MirrorSet(self, orig = None, new = None, addl = None):
        if type(orig) is not str:
            raise TypeError('orig list is a required argument')
        if type(new) is not str:
            raise TypeError('new is a required argument')

Again, don't provide defaults and then reject them. Python can do a perfectly good job of rejecting missing parameters. Also, don't check types. Document what the types are supposed to be, but let trust the user of your code.

        try:
            files = copy.deepcopy(self.file_set[orig]['files'])

Why are you making a deepcopy?

            skin_path = os.path.join(self.root, new)
            out = os.path.join(skin_path, orig)
            for i, filename in enumerate(files):
                filePath = os.path.join(skin_path, filename)
                if (os.path.exists(filePath)):
                    files[i] = os.path.join(new, files[i])

Don't modify the list you are iterating over, create a new list.

            if addl is not None and type(addl) is list:
                files.extend(addl)

Don't check types, and especially don't ignore incorrect types

            self.file_set[out] = {'files' : files, 'callbacks' : self.file_set[orig]['callbacks']}
        except:
            raise

Don't catch an exception just to re-raise it.

    def stop(self):
        self.observer.stop()

    def start(self):
        event_handler = ChangeHandler(self._on_file_changed)
        self.observer = Observer()
        self.observer.schedule(event_handler, self.root, recursive=True)
        self.observer.start()

    def __init__(self, root = None):
        if type(root) is not str:
            raise TypeError('root is required as argument in object instantation')

DON'T!

        self.root = os.path.abspath(root)