patternpythonMinor
Monitor filesystem for continuous integration and build
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']):
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.rootYou 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 pathLook 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 rvlogging.info('each callback asked us to exit quietly')
returnThrow 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.rootif path.startswith(root):
path = path[len(root):]
if path.startswith('/'):
path = path[1:]
return pathContext
StackExchange Code Review Q#21639, answer score: 4
Revisions (0)
No revisions yet.