HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Monitor filesystem for continuous integration and build

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
forintegrationmonitorandfilesystembuildcontinuous

Problem

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'):
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']):

Solution

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')
logging.info("Wrote file: " + out)
if hasattr(callbacks, 'post') and callable(callbacks['post']):
callbacks'post'

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(se

Code Snippets

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":
if not event.is_directory and event.event_type == modified:
     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

Context

StackExchange Code Review Q#21639, answer score: 4

Revisions (0)

No revisions yet.