getMail (fetchmail-ersatz)

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
lunqual
User
Beiträge: 2
Registriert: Sonntag 8. Februar 2009, 13:57
Wohnort: Gutach, Baden-Württemberg

getMail (fetchmail-ersatz)

Beitragvon lunqual » Sonntag 8. Februar 2009, 14:07

Moin zusammen.

Ich möchte hier mal ein kleines Projekt vorstellen.
getMail ist ein fetchmail-ersatz, welches mails für beliebig viele Benutzer über POP3 abholt und über SMTP weiterleitet.
Ich will dabei aber nicht die Frage diskutieren, ob das überhaupt sinnvoll ist, weil ja fetchmail funktioniert....
Ich hatte durchaus einen Grund, das so zu machen...

Wie auch immer, im Zweifelsfalle ist es auch lehrreich, sowas zu machen, auch und gerade für mich, der ich in Python nicht so bewandert bin.

Ich würde mich über Kommentare und Beurteilungen freuen, speziell dazu, ob ich irgendwas anders machen, kürzer oder mehr pythonic machten könnte, und auch natürlich darüber, ob das Projekt einigermassen gut strukturiert oder nicht ist.

Ihr könnt das Archiv (ca. 8kb) hier Herunterladen

http://www.lunqual.de/getmail-08022009.tar.bz2


Es enthält ein README mit dem wichtigsten zur Ausführung und eine einigermassen kommentierte Konfigurationsdatei.

Viel Spaß und danke schonmal im voraus für eure Mühe ;))
DasIch
User
Beiträge: 2404
Registriert: Montag 19. Mai 2008, 04:21
Wohnort: Berlin

Beitragvon DasIch » Sonntag 8. Februar 2009, 14:50

Du solltest dich an PEP 8 halten und ein paar Docstrings könnten nicht Schaden ;) Argumente wie `verbose` sollte man imho optional (über keywords) übergeben können.

Wieso hast du eigentlich den ganzen eric4 Kram mit im Archiv? Zumindest zur Veröffentlichung würde ich den weg lassen.
BlackJack

Beitragvon BlackJack » Sonntag 8. Februar 2009, 19:43

@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.
audax
User
Beiträge: 830
Registriert: Mittwoch 19. Dezember 2007, 10:38

Beitragvon audax » Sonntag 8. Februar 2009, 23:40

Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Montag 9. Februar 2009, 01:16

audax hat geschrieben:http://pyropus.ca/software/getmail/

:o

Ist in Python und funktioniert übrigens sehr gut, habe ich selbst eine zeitlang benutzt.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
lunqual
User
Beiträge: 2
Registriert: Sonntag 8. Februar 2009, 13:57
Wohnort: Gutach, Baden-Württemberg

Beitragvon lunqual » Montag 9. Februar 2009, 08:45

Vielen Dank für deine sehr ausführlichen Bemerkungen.
Wie gesagt, ich kenn mich mit Python nicht so sehr aus, deswegen frag ich ja...

Zu den Bemerkungen:

> Wofür ist `getmail.gml` da?
Keine Ahnung, ist wohl "übriggeblieben"

>Die Klasse `getmail` ist überflüssig, das hätte man auch mit Funktionen machen können.

Ich dachte, dass man Klassen einfachen Funktionen vorziehen sollte ?? Ein Fehler isses jedenfalls nicht, oder ?

> Vergleiche mit `True` oder `False` sind stilistisch unschön. ``if verbose == True:`` sagt nicht mehr aus als ``if verbose:``.

Das ist Ansichtssache. Ich jedenfalls ziehe das Ausgeschriebene vor, einfach der Klarheit wegen.

> Bei `getConfig()` verdeckst Du das eingebaute `file`.

Wusste ich nicht, werd das näxtemal dran denken

> Den Test ob die Datei existiert, kann man sich sparen.

Hmmm... Ich bin halt irgendwie "vom alten Schlag" und mach das gewohnheitsmässig so. Ich war bisher der Meinung, dass ein solches Vorgehen "sicherer" ist...

> Warum wird `getmail_get` als externes Programm gestartet und nicht importiert und verwendet?

Der Grund ist folgender:
Zum ersten wird das Programm mit ca. 30 Mailkonten verwendet, so dass für jedes Mailkonto ein eigener Prozess gestartet wird, wodurch die Verarbeitung in ein paar Sekunden komplett erledigt ist.
Zum zweiten bin ich mir bewusst, dass man dafür auch 30 Threads starten könnte, aber das ist mir als Anfänger in Python im Moment zu kompliziert.
Im übrigen habe ich aus dem Lesen den Eindruck gewonnen, dass Threads immer auf dem gleichen Prozessorkern gestartet werden, "Programme" aber möglicherweise auf mehrere Kerne verteilt werden. Ich dachte halt, dass ich die 8 Kerne im Server so besser ausnutzen kann.
Sollte dieser Gedanke flasch sein, wäre ich an einer Erklärung interessiert.

Im ganzen übrigen Teil machst du sehr konkrete Vorschläge, die mir die Funktionsweise von Python durchaus näherbringen.

Jedenfalls nochmals Danke für die ausführliche Antwort, jetzt hab ich erstmal was zum nachdenken...
Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Montag 9. Februar 2009, 09:20

lunqual hat geschrieben:>Die Klasse `getmail` ist überflüssig, das hätte man auch mit Funktionen machen können.

Ich dachte, dass man Klassen einfachen Funktionen vorziehen sollte ?? Ein Fehler isses jedenfalls nicht, oder ?

Nein, in Python nutzt man Klassen wo es nötig ist, statt Klassennutzung zu erzwingen. Die Benutzung von Klassen einfach nur um Klassen zu verwenden ist ziemlicher Quark und wohl auch der offensichtlichste Designfehler in Java.

lunqual hat geschrieben:> Vergleiche mit `True` oder `False` sind stilistisch unschön. ``if verbose == True:`` sagt nicht mehr aus als ``if verbose:``.

Das ist Ansichtssache. Ich jedenfalls ziehe das Ausgeschriebene vor, einfach der Klarheit wegen.

Dann bist du in der Minderheit. Ohne das ``== False`` gibt es nocht weitere vorteile: So ist auch eine leere Liste und ein leerer String bei so einem ``if`` False, also kannst du diese drei Fälle alle mit dem gleichen Code erschlagen.

lunqual hat geschrieben:> Den Test ob die Datei existiert, kann man sich sparen.

Hmmm... Ich bin halt irgendwie "vom alten Schlag" und mach das gewohnheitsmässig so. Ich war bisher der Meinung, dass ein solches Vorgehen "sicherer" ist...

In Python verwendet geht man öfter nach EAFP (Easier to ask forgiveness than permission) vor. Gerade im Zugriff auf Dateien spart man sich da eventuelle Race-Conditons da zwischen den Existenzcheck und dem Zugriff auf die Datei ein anderer Thread die Datei löschen könnte.

lunqual hat geschrieben:> Warum wird `getmail_get` als externes Programm gestartet und nicht importiert und verwendet?

Der Grund ist folgender:
Zum ersten wird das Programm mit ca. 30 Mailkonten verwendet, so dass für jedes Mailkonto ein eigener Prozess gestartet wird, wodurch die Verarbeitung in ein paar Sekunden komplett erledigt ist.

Ja, aber wenn du das in eine ``main()`` packst, könnten andere Programmierer die API deines Modules für eigene Programme nutzen. Wenn man es aus der Konsole startet, kann man ja die ``main()`` aufrufen lassen.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
audax
User
Beiträge: 830
Registriert: Mittwoch 19. Dezember 2007, 10:38

Beitragvon audax » Montag 9. Februar 2009, 10:08

Leonidas hat geschrieben:
audax hat geschrieben:http://pyropus.ca/software/getmail/

:o

Ist in Python und funktioniert übrigens sehr gut, habe ich selbst eine zeitlang benutzt.

Bei mir ist es seit Jahren im Dauereinsatz :D

getmail ftw!

Zum "if foo == True":
Explizit is better than implizit sollte nicht überttrieben werden. Ein "foo == True" gibt wieder ein True oder False zurück, also sollte man der logik nach dieses True wieder
LOOP: testen:

Code: Alles auswählen

if (foo == True) == True:

Nur gibt das wieder nur ein True oder False züruck und man müsste
goto LOOP
EyDu
User
Beiträge: 4866
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Beitragvon EyDu » Montag 9. Februar 2009, 11:32

Nur um sicher zu gehen :) :

Code: Alles auswählen

import itertools
import operator
def has_value(x, value=True):
    return reduce(operator.eq, itertools.chain([x, value], itertools.reapeat(True)))
Das Leben ist wie ein Tennisball.
BlackJack

Beitragvon BlackJack » Dienstag 10. Februar 2009, 09:22

@lunqual: Bei Programmen "vom alten Schlag" musste man immer explizit vorher auf Fehlerbedingungen prüfen, weil die verwendeten Sprachen keine Ausnahmen kannten. Wenn man vorher prüft und Ausnahmen behandelt, macht man im Grunde die Arbeit doppelt.

So kompliziert sind Threads mit dem `threading`-Modul nicht, vor allem, wenn sie sich, wie in diesem Fall, keine Datenstrukturen teilen, die verändert werden.

Das stimmt schon, dass die Threads, was Python-Bytecode angeht, bei CPython nur auf einem Prozessor(kern) laufen, aber die Aufgabe ist sehr I/O-lastig, da bringt das schon Vorteile gegenüber einer sequentiellen Abarbeitung.

Ansonsten kann man ab Python 2.6 das Modul `multiprocessing` aus der Standardbibliothek verwenden, was es für ältere Python-Versionen als `processing`-Modul gibt: http://pyprocessing.berlios.de/

Wer ist online?

Mitglieder in diesem Forum: 0 Mitglieder