@erselbst: Die Dokumentation zu Packages sagt ja nicht das man jedes Projekt unbedingt irgendwie auf Packages und Unterpackages verteilen soll, sondern Dokumentiert was Packages und Unterpackages sind. Ich habe es ja schon geschrieben: IMHO ist Dein Programm jetzt schon zu stark aufgeteilt. Eine Klasse pro Modul macht keinen Sinn, weil dann das Modul als Organisationseinheit wertlos wäre. Ein Modul ist dazu da zusammengehörige Konstanten, Funktionen, und Klassen zusammenzufassen. Das könnte man also alles in *einem* Modul speichern.
An dem Quelltext gibt es einige etwas komische Sachen und einiges was zu umständlich gelöst ist.
`SyncException` bräuchte einfach nur aus einem ``pass`` bestehen und man hätte den selben Effekt. Die Ausnahme scheint einfach nur benutzt zu werden um alle anderen Ausnahmen die abgefangen werden durch diese weniger spezifische Ausnahme zu ersetzen. Das ist IMHO keine sinnvolle Ausnahmebehandlung.
`SyncLogger` macht keinen Sinn. Wozu hat man Loglevels wenn man in jeder Methode das Level der jeweiligen Methode einstellt. Da könnte man einfach einmal `DEBUG` setzten und dann die ganz normalen Methoden des Objekts verwenden. Damit werden ausser der `__init__()` alle anderen Methoden überflüssig. Und wenn man nur die `__init__()` hat, macht eine eigene Klasse dafür nicht mehr viel Sinn, da kann man auch einfach eine Funktion schreiben, welche die Logging-Einstellungen vornimmt und einen Logger zurück gibt.
Der `ConfigReader` erscheint mir eine übermässig komplexe Kapselung des `ConfigParser`zu sein. Da wird das `ConfigParser`-Exemplar unnötig an das Objekt gebunden, weil es nach der Abarbeitung von `__init__()` überhaupt nicht mehr gebraucht wird. Und die verschachtelte Struktur einer INI-Datei wird auf Attribute aufgeteilt, nur um beim Zugriff dann Entscheidungen mit ``if``\s zu treffen, die nur deswegen überhaupt nötig sind. Der einzige Mehrwert ist die Möglichkeit eine Liste von Werten abzufragen. Dazu hätte man aber auch vom `ConfigParser` ableiten und eben genau diese eine zusätzliche `get`-Methode implementieren können. Die gesamte Klasse würde damit auf drei einfache Zeilen schrumpfen.
Die Mehrzahl von „directory” heisst übrigens „directories”.
Dateien die man öffnet, sollte man übrigens auch wieder schliessen. Die ``with``-Anweisung kann helfen das zuverlässig zu erledigen.
Mit `CommandExec` haben wir wieder eine sinnlose Klasse, die problemlos durch eine einfachere Funktion ersetzt werden kann. Das ist fast immer bei Klassen der Fall die neben der `__init__()` nur eine Methode haben, die dann auch noch genau einmal direkt nach dem Erstellen des Exemplars aufgerufen wird. Das `config`-Argument wird überhaupt nicht verwendet.
``shell=True`` sollte man bei `Popen` nicht verwenden, schon gar nicht wenn man keine Kontrolle über die Argumente hat die da irgendwie in die Kommandozeichenkette hinein gebastelt wurden. Wenn man nicht ganz so viel Pech hat, dann haut nur das Escapen von Shell-Sonderzeichen nicht richtig hin. Wenn man aber so richtig Pech hat, nutzt das jemand aus um beliebigen Shell-Code mit den Rechten dieses Prozesses ausführen zu lassen. Wenn Du die Ausgabe ignorierst, warum wird sie dann überhaupt vom Programm aufgefangen?
Logging wird falsch verwendet. Das zeichnete sich ja schon bei der eigenartigen Logging-Klasse ab. Beim Logging wird nicht überall im Code entschieden ob geloggt werden soll oder nicht, sondern es wird grundsätzlich die Loggingmethode aufgerufen und das eingestellte Loglevel entscheidet darüber ob und wie das dann ausgegeben wird.
Den `SyncEventHandler` kann man etwas kürzen in dem man die Namen der speziellen Methoden an die `proc()`-Methode bindet statt drei Methoden zu schreiben, die einfach nur `proc()` mit den gleichen Argumenten aufrufen.
Die `notify()`-Methode ist zu lang. Das dürfte zum Teil von Kopieren und Einfügen kommen, statt die Gemeinsamkeiten in eine Funktion auszulagern. Man könnte das auch in eine eigene Klasse kapseln, und bestimmte Prüfungen und das zusammensetzen des Kommandos *einmal* machen, statt bei jedem Aufruf aufs neue.
In der `run()`-Methode wird `watchdir` unnötigerweise an das Objekt gebunden. Die `_watchdir()`-Methode ist überkompliziert und ineffizient. Hier scheint eigentlich nur `str.startswith()` nötig zu sein.
Das ``try``/``except``-Konstrukt in der `main()`-Funktion macht eigentlich keinen Sinn, denn es ersetzt die Ausnahme nur durch eine weniger Informative die noch nicht einmal einen Traceback besitzt, also nicht gerade hilfreich bei der Fehlersuche ist.
Ungetestet und IMHO immer noch verbesserungswürdig:
Code: Alles auswählen
#!/usr/bin/env python
# -*- coding: utf-8 -*-
import logging
import os
import re
import sys
from ConfigParser import ConfigParser
from subprocess import Popen, STDOUT
import pyinotify
LOGFORMAT = '%(asctime)s %(levelname)s %(message)s'
MASK = pyinotify.IN_DELETE | pyinotify.IN_CLOSE_WRITE | pyinotify.IN_MOVED_TO
LOG = logging.getLogger('synclogger')
def configure_logger(filename, level):
file_handler = logging.FileHandler(filename, 'a')
file_handler.setFormatter(logging.Formatter(LOGFORMAT))
LOG.addHandler(file_handler)
LOG.setLevel(level)
class ExtendetConfigParser(ConfigParser):
def get_strings(self, section, option):
return [s.strip() for s in self.get(section, option).split(',')]
def execute_command(command, out_file=None):
process = Popen(
command, stderr=STDOUT if out_file else None, stdout=out_file
)
process.wait()
LOG.info(
'The command %s has completed with exit code %s.',
command,
process.returncode
)
return process.returncode
class SyncEventHandler(pyinotify.ProcessEvent):
# Exclude filter
match_exclude = re.compile(r'.+(\.sw[xp]|~)$').match
def __init__(self, config, pevent=None, **kwargs):
pyinotify.ProcessEvent.__init__(self, pevent, **kwargs)
self.config = config
def process(self, event):
if not self.match_exclude(event.pathname):
LOG.info(
'The event %s has been applied to %s.',
event.maskname,
event.pathname
)
if self.run(event) == 0:
self.notify('success', event)
else:
self.notify('failure', event)
process_IN_DELETE = process_IN_CLOSE_WRITE = process_IN_MOVED_TO = process
def notify(self, state, event):
notify_time = '%.0f' % (
self.config.getint('globals', 'notify_delay') * 1000
)
if state == 'success':
command = [
self.config.get('globals', 'notify_binary'),
'-t', notify_time,
]
if os.path.isfile(
self.config.get('globals', 'notify_icon_success')
):
command.extend(
['-i', self.config.get('globals', 'notify_icon_success')]
)
notify_text = self.config.get('messages', 'msg_success_sync') + ' '
if event.maskname == 'IN_DELETE':
notify_text += self.config.get('messages', 'msg_delete')
else:
notify_text += self.config.get('messages', 'msg_upload')
command.extend(
[
self.config.get('globals', 'notify_subject'),
notify_text % event.name
]
)
execute_command(command)
if (
os.path.isfile(
self.config.get('globals', 'notify_sound_success')
)
and os.path.isfile(self.config.get('globals', 'mplayer_binary'))
):
execute_command(
[
self.config.get('globals', 'mplayer_binary'),
self.config.get('globals', 'notify_sound_success')
]
)
elif state == 'failure':
command = [
self.config.get('globals', 'notify_binary'),
'-t', notify_time,
]
if os.path.isfile(
self.config.get('globals', 'notify_icon_failure')
):
command.extend(
['-i', self.config.get('globals', 'notify_icon_failure')]
)
command.extend(
[
self.config.get('globals', 'notify_subject'),
self.config.get('messages', 'msg_failure_sync')
]
)
execute_command(command)
if (
os.path.isfile(
self.config.get('globals', 'notify_sound_failure')
)
and os.path.isfile(self.config.get('globals', 'mplayer_binary'))
):
execute_command(
[
self.config.get('globals', 'mplayer_binary'),
self.config.get('globals', 'notify_sound_failure')
]
)
else:
LOG.error('Unknown state %r', state)
def run(self, event):
watchdir = self._watchdir(event)
if watchdir is not None:
with open(
self.config.get('globals', 'sync_logfile'), 'w'
) as log_file:
return execute_command(
[
self.config.get('globals', 'sync_binary'),
self.config.get('globals', 'sync_options'),
'-e', 'ssh -i {0} -p {1}'.format(
self.config.get('globals', 'sync_sshkeys'),
self.config.get('globals', 'sync_sshport')
),
watchdir,
self.config.get('globals', 'sync_targets'),
self.config.get('globals', 'sync_logfile'),
],
log_file
)
else:
return 99 # TODO: Too magic.
def _watchdir(self, event):
for directory in self.config.get_strings('directories', 'directories'):
if re.compile(r'^'+os.path.join(self.config.get('globals', 'base'), directory)+os.path.sep).match(event.pathname):
return os.path.join(self.config.get('globals', 'base'), directory)
return None
def main():
if len(sys.argv) < 2:
sys.stderr.write(
'An Error occured:'
' You have to give a config file as argument.\n'
)
sys.exit(1)
config = ExtendetConfigParser()
config.read(sys.argv[1])
configure_logger(
config.get('globals', 'logfile'),
logging.DEBUG
if config.getboolean('globals', 'logging')
else logging.ERROR
)
watch_manager = pyinotify.WatchManager()
notifier = pyinotify.Notifier(watch_manager, SyncEventHandler(config))
for directory in config.get_strings('directories', 'directories'):
watch_manager.add_watch(
os.path.join(config.get('globals', 'base'), directory),
MASK,
rec=True
)
notifier.loop()
if __name__ == '__main__':
main()
Ich würde das herumreichen vom Logger und von der Konfiguration einschränken.
Edit: Logger nicht mehr herum gereicht.