@lunqual: Welche Python-Version verwendest Du? Zeichen ausserhalb von ASCII ohne Kodierungskommentar werden von einigen Python-Versionen mit einem `SyntaxError` quittiert.
Wofür ist `getmail.gml` da? Warum werden `config` und `logger` als Klassenattribute definiert?
Docstrings gehören unter die definierenden Anweisungen wie ``class`` oder ``def``.
Klassennamen werden per Konvention in `CamelCase` geschrieben.
Die Klasse `getmail` ist überflüssig, das hätte man auch mit Funktionen machen können.
Vergleiche mit `True` oder `False` sind stilistisch unschön. ``if verbose == True:`` sagt nicht mehr aus als ``if verbose:``.
Abgekürzte Namen sind nicht gut. Das `fh` kann man sich noch denken, aber bei `ch` fällt mir auf Anhieb nichts ein.
Bei `getConfig()` verdeckst Du das eingebaute `file`. Genau wie bei `logSetup()`\s `logfile` ist der Name auch nicht ganz passend, da es sich nicht um Dateien, sondern um Dateinamen handelt.
Den Test ob die Datei existiert, kann man sich sparen. Im Ernstfall kann es sogar passieren, dass der Test positiv verläuft und zwischen Test und Leseversuch die Datei entfernt wird. Die `ConfigParser.read()`-Methode gibt eine Liste mit erfolgreich gelesenen Konfigurationsdateinamen zurück. Man könnte das zum Beispiel auch so schreiben:
Code: Alles auswählen
def get_config(self, filename):
"""Konfigurationsdatei laden.
"""
self.logger.debug('Loading config File %s' % filename)
result = ConfigParser.ConfigParser()
try:
if not result.read(filename):
return result
else:
self.logger.error('Config File %s not found' % filename)
except ConfigParser.MissingSectionHeaderError:
self.logger.error('Config file has no Sections')
return None
Wobei "spezielle" Rückgabewerte für Fehlersituationen sehr unschön sind. Genau um so etwas loszuwerden wurden Ausnahmen erfunden.
Warum wird `getmail_get` als externes Programm gestartet und nicht importiert und verwendet?
In Zeile 82 wird ein Tupel erzeugt aber nicht an einen Namen gebunden oder anderweitig verwendet!?
Zu `getmail_get`:
Die Klasse `getmail_get` hat auch wieder dieses mysteriöse `gml`-Attribut!? Und auch diese Klasse scheint wieder nur als Namensraum missbraucht zu werden!? Veränderliche Strukturen wie `daten` (schlechter Name), `blacklist`, und `whitelist` sollten keine Klassenattribute sein. Dadurch kann man nicht mehrere Exemplare gleichzeitig verwenden, womit die Klasse nicht sinnvoll ist.
`logSetup()` und `getConfig()` sind identisch zu denen im `getmail`-Modul, d.h. diese beiden Funktionen sollte man nur einmal in *einem* Modul haben.
`getLock()` ist unsicher, da es durchaus passieren kann, dass zwei Prozesse auf die Existenz der Datei prüfen, beide feststellen, dass sie noch nicht da ist und dann beide Versuchen sie anzulegen und einer die Sperrdatei des anderen überschreibt. Für so etwas braucht man eine atomare Funktion, wie zum Beispiel das Anlegen eines Verzeichnisses (unter Unix zumindest). Ohne vorherigen Test.
Der Test auf `isfile()` in `unLock()` ist im Grunde auch wieder überflüssig.
`i` sollte man nur für ganze Zahlen verwenden. Alles andere ist sehr verwirrend.
Das innerste ``try``/``except`` in `getValues()` verstehe ich nicht. Es ist schon bezeichnend, das dort keine explizite Ausnahme behandelt wird, was sollte denn bei der Zeile schief laufen? Und vor allem warum sollte `i` nicht "gefunden" werden, wo es doch an der Stelle 100%ig an einen Schlüssel auf der Konfiguration, Abschnitt `section` gebunden ist!? Der Methodenname impliziert ausserdem, dass dort Werte zurückgegegeben werden.
Die Bedingung einer ``if``-Anweisung ist schon ein Wahrheitswert, da braucht man kein ``if``/``else`` um `True` oder `False` zurück zu geben, wie in `getBoolean()`:
Code: Alles auswählen
def string2bool(string):
return string.strip().lower() in ('yes', '1', 'true')
Bei `getWhitelist()` und `getBlacklist()` suggeriert der Name wieder, dass da eine Liste zurückgegeben wird. Beide enthalten ausserdem fast identischen Quelltext, den man zu einer Funktion zusammenfassen kann:
Code: Alles auswählen
def get_list(config, section):
try:
return sorted(config.items(section))
except ConfigParser.NoSectionError:
self.logger.error('Section [%s] not found' % section)
return []
`getFrom()` ist eine Funktion und keine Methode. Der Fall, dass 'from:' gar nicht als Zeilenanfang vorkommt, sollte mit einer Ausnahme beantwortet werden.
Das Argument `headerlines` wird in `getTo()` gar nicht verwendet.
Warum wird in `isBlacklisted()` nichts mit `item` gemacht? Die Namen `item` und `value` sind hier auch zu generisch. Statt `re.compile()` könnte man entweder `re.search()` verwenden, oder die regulären Ausdrücke *einmal* kompilieren und immer wieder verwenden.
`isWhitelisted` ist fast identisch mit `isBlacklisted()` -> zusammenfassen.
`isOk()` ist wieder so ein Schmuckstück, das viel zu kompliziert geschrieben ist.
Code: Alles auswählen
def isOk(self, headerlines):
return self.isWhitelisted(headerlines) or not isBlacklisted(headerlines)
``mail[:headerlines]`` wird in `getSingle()` drei mal gemacht. In der Methode wird zu viel gemacht, und auch viel zu viel Arbeit unnötigerweise immer wieder gemacht. `self.daten` ist sehr eigentartig. Das sollte einmal ordentlich aufbereitet und als Attribute mit aussagekräftigen gespeichert werden.
Die erste Zeile in `checkMail()` ist überflüssig.
Insgesamt sollte man das Programm deutlicher in Richtung Funktionen oder OOP entwicklen und nicht nahezu statische Klassen als Modul im Modul missbrauchen. Und die Funktionen/Methoden mehr am Datenfluss ausrichten und in Jeder nur *eine* abgegrenzte Tätigkeit durchführen, dann wird man diese tiefen ``if``/``else``-Verschachtelungen und Hilfsvariablen mit Wahrheitswertswerten los.
Ein paar Zeilen sind länger als 80 Zeichen.
)
: