Bewertung meines ersten ernst gemeinten Python-Codes

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Moin,
wie ich an anderer Stelle schon erwähnt habe, ist Python ziemliches Neuland für mich. Jetzt habe ich mich mal rangesetzt und angefangen mein erstes ernsthaftes Projekt damit zu machen.
Nunja, das Projekt existiert eigentlich schon, ist ein Programm das im Betrieb von qmail einige Hilfestellungen bietet und ist bislang in Perl geschrieben, leider aber über eine lange Zeit immer weiter organisch gewachsen (will heißen: eine neue Funktion einzubauen ist ein echter Krampf).
Da ich das ganze ohnehin neu auflegen wollte, tue ich das nun in Python statt in Perl (mal sehen, ob das eine Weise Entscheidung war ;)). So richtig weit bin ich heute erwartungsgemäß nicht gekommen, da ich mich mehr mit der Dokumentation etc. rumgeschlagen habe. Aber ich habe zumindest einiges das schon funktioniert.

Stellvertretend habe ich eine Methode ausgewählt, die dafür zuständig ist, zu checken ob eine Empfängeradresse auf dem Server ernst zu nehmen ist.
Nun möchte ich mal eure Meinung dazu hören, wissen was ich anders/besser/pythoniger machen sollte usw.

Ich befürchte, dass ich zu viele Klammern und Reguläre Ausdrücke (kommt halt aus Perl ;)) benutzt habe, um mich als Pythoner zu fühlen.

Code: Alles auswählen

def isValidRecipient(self, recipient):
	addresses = recipient.split(",")

	emailPattern = "[-_a-z0-9]+@[-.a-z0-9]+\.[a-z]{2,6}"
	addrRE = re.compile("^&?" + emailPattern + "$")
	longAddrRE = re.compile("^.*<(" + emailPattern + ")>")

	for addr in addresses:
		addr = addr.replace("&", "", 1)
		addr = addr.strip()

		# recipient might be in format "John Doe <j.doe@example.com>"
		addr = longAddrRE.sub(r"\1", addr)

		parts = addr.split("@")
		localpart = parts[0]
		domain = parts[1]

		if(os.path.isdir("/var/vpopmail/domains/" + domain + "/" + localpart + "/Maildir")):
			return True
		
		# is this a forward-address?
		aliasPath = "/var/vpopmail/domains/" + domain + "/.qmail-" + localpart
		if(os.path.isfile(aliasPath)):
			aliases = open(aliasPath)
			for line in aliases:
				line = line.strip()
				if(addrRE.search(line) is not None):
					if(self.isValidRecipient(line)):
						return True
				else:
					return True

			return False

		# is this a forwared recipient in format "domain-localpart@domain?
		aliasRE = re.compile("^" + domain + "-")
		localForward = aliasRE.sub(r"", localpart)
		if(localForward != localpart):
			if(self.isValidRecipient(localForward + "@" + domain)):
				return True
	return False
BlackJack

Schau Dir mal den Style Guide an was die Namensgebung betrifft.

Um Bedingungen bei ``if``s braucht's keine Klammern.

Soweit ich das auf den ersten Blick sehen kann, wird `self` nicht wirklich benutzt, das ganze könnte also eine Funktion sein.

`parts` braucht man nicht wirklich:

Code: Alles auswählen

localpart, domain = addr.split('@')
Pfadnamen sollte man mit `os.path.join()` zusammensetzen. Dateien sollte man explizit schliessen.

Ich persönlich würde es auf mehrere Funktionen aufteilen, angefangen bei einer, die die Adressliste aufarbeitet bis zu Funktionen die die einzelnen Teilaspekte überprüfen. Und vielleicht weniger REs benutzen. :-)
Benutzeravatar
Luzandro
User
Beiträge: 87
Registriert: Freitag 21. April 2006, 17:03

Beim email-Pattern müsste in der ersten Gruppe zumindest noch ein "." vorkommen und das ganze sollte noch case-insensitive sein.

Code: Alles auswählen

addr = longAddrRE.sub(r"\1", addr)
ist zwar korrekt und kürzer, ich persönlich würde es aber dennoch anders schreiben:

Code: Alles auswählen

addr = longAddrRE.search(addr).group(1)
[url=http://www.leckse.net/artikel/meta/profilieren]Profilieren im Netz leicht gemacht[/url]
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

BlackJack hat geschrieben:Schau Dir mal den Style Guide an was die Namensgebung betrifft.
Ohje, das nenne ich mal einen gewöhnungsbedürftigen Stil, den ihr Pythoner da pflegt ;)
Na, ich werd mich mal daran orientieren.
Um Bedingungen bei ``if``s braucht's keine Klammern.
Okay
Soweit ich das auf den ersten Blick sehen kann, wird `self` nicht wirklich benutzt, das ganze könnte also eine Funktion sein.
Naja, es soll schon Teil einer Klasse sein. Statische Methoden gibt's ja nicht, aber im nach dem Styleguide hätte ich vielleicht statt self eher cls nehmen?
`parts` braucht man nicht wirklich:

Code: Alles auswählen

localpart, domain = addr.split('@')
Feine Sache
Pfadnamen sollte man mit `os.path.join()` zusammensetzen. Dateien sollte man explizit schliessen.
Warum? Ich glaub dir, dass es da sinnvolle Gründe für gibt, aber ich bin mir im Moment nicht ganz sicher, warum das in diesem Fall sinnvoll wäre bzw. wie das dann aussehen sollte.

Code: Alles auswählen

os.path.join("var", "vpopmail", "domains", domain, localpart, "Maildir")
os.path.join("/var", "vpopmail", ...)
os.path.join("/var/vpopmail/domains", domain, localpart, "Maildir");
Ich persönlich würde es auf mehrere Funktionen aufteilen, angefangen bei einer, die die Adressliste aufarbeitet bis zu Funktionen die die einzelnen Teilaspekte überprüfen. Und vielleicht weniger REs benutzen. :-)
Das mit dem Aufteilen ist wohl war, hab mich recht dicht an der Perl-Version gehalten, was sicherlich von Nachteil ist.
Auf die meisten REs fällt es mir doch eher schwer zu verzichten.
Beim email-Pattern müsste in der ersten Gruppe zumindest noch ein "." vorkommen und das ganze sollte noch case-insensitive sein.
Stimmt, das Pattern ist noch verbesserungswürdig. Ich habe gestern nichts gefunden, wie ich einen Regulären Ausdruck mit Modifiern wie "i" versehen kann.
Deine alternative Schreibweise für die Ersetzung hat wirklich was für sich.


Herzlichen Dank schonmal an euch beide :)
Mal sehen, ob ich heute dazu komme, eine überarbeitete Version zu schaffen.[/code]
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

dst hat geschrieben:Statische Methoden gibt's ja nicht, aber im nach dem Styleguide hätte ich vielleicht statt self eher cls nehmen?
Statische Methoden gibts schon und ``cls`` ist schon für Metaklassen besetzt, wo das IMHO auch sinniger ist.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

dst hat geschrieben:Stimmt, das Pattern ist noch verbesserungswürdig. Ich habe gestern nichts gefunden, wie ich einen Regulären Ausdruck mit Modifiern wie "i" versehen kann.
Etwa so:
``re.compile(<deine-regex>, re.IGNORECASE)``
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Hmm,
für statische Methoden habe ich nur Lösungen gefunden, die mir sehr nach merkwürdigen Workarounds aussehen. Sowas in der Art:

Code: Alles auswählen

class SomeClass:
	class StaticMethod:
		def __call__(self):
			# mach was tolles

		static_method = StaticMethod()

SomeClass.static_method()

Zu Luzandros RE-Verbesserung:
Diese Methode:

Code: Alles auswählen

addr = longAddrRE.search(addr).group(1)
hat einen Nachteil. Gibt es keinen Treffer, gibt search() None zurück, was bedeutet, dass group() nicht aufgerufen werden kann.
Es müsste dann schon eher so heißen:

Code: Alles auswählen

foo = longAddrRE.search(addr)
if foo is not None:
	addr = foo.group(1)
Benutzeravatar
Luzandro
User
Beiträge: 87
Registriert: Freitag 21. April 2006, 17:03

dst hat geschrieben:Zu Luzandros RE-Verbesserung:
Diese Methode:

Code: Alles auswählen

addr = longAddrRE.search(addr).group(1)
hat einen Nachteil. Gibt es keinen Treffer, gibt search() None zurück, was bedeutet, dass group() nicht aufgerufen werden kann.
Es müsste dann schon eher so heißen:

Code: Alles auswählen

foo = longAddrRE.search(addr)
if foo is not None:
	addr = foo.group(1)
Oh, stimmt - dachte du willst damit immer nur die erste Gruppe bekommen. Aber eigentlich ist diese longAddrRE sowieso unnötig, denn das einzige was du willst ist das emailPattern aus diesem addr-String herausfischen:

Code: Alles auswählen

re.search(emailPattern, addr).group(0)
Das strip brauchst du daher auch genausowenig und bei dem "&"-replace weiß ich jetzt nicht wofür das gebraucht wird, aber nachdem es in deinem addrRE vor dem emailPattern vorkommt, hat es wohl auch nichts mit der Adresse zu tun
[url=http://www.leckse.net/artikel/meta/profilieren]Profilieren im Netz leicht gemacht[/url]
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Da hast Du recht, eigentlich kann ich das umdrehen.

Das & kann in den qmail-Konfigurationsdateien vor eine Adresse stehen, ist für meine Belange aber ohne Bedeutung bzw. störend.
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

dst hat geschrieben:für statische Methoden habe ich nur Lösungen gefunden, die mir sehr nach merkwürdigen Workarounds aussehen. Sowas in der Art:

Code: Alles auswählen

class SomeClass:
	class StaticMethod:
		def __call__(self):
			# mach was tolles

		static_method = StaticMethod()

SomeClass.static_method()
Also ich finde ja

Code: Alles auswählen

In [38]: class MyStatic(object):
   ....:     @staticmethod
   ....:     def static_method():
   ....:         print 'Bin grad dabei was tolles zu machen'
   ....:
In [39]: MyStatic.static_method()
Bin grad dabei was tolles zu machen
durchaus elegant. Nicht das ich Static Methods besonders oft brauche.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Da muss ich mir wohl noch ein bisschen was angucken. Unter was für einem Begriff findet man das mit dem @ denn oder hast Du gar einen tollen Link für mich? :)

Aber im Prinzip braucht man statische Methoden unter Python wohl noch seltener als unter anderen Sprachen, weil man sie meist wohl genausogut als Funktion direkt ins jeweilige Modul setzen könnte.
Wenn mich mein bisheriges Python-Verständnis nicht trügt, dürfte das doch vom Prinzip her eine statische Methode auf Modulebene sein, während man ein Modul auch mit einer statischen Klasse vergleichen könnte.
Hmm, so richtig rund ist der Gedankengang noch nicht. Verdammt, ist schon eine Welt für sich.
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

dst hat geschrieben:Da muss ich mir wohl noch ein bisschen was angucken. Unter was für einem Begriff findet man das mit dem @ denn oder hast Du gar einen tollen Link für mich? :)
Das ist die Dekorator-Syntax, dazu gibt es den PEP 318, eine Einführung von PJE der schon ganz verwunderliche Dinge damit angestellt hat, eine kurze EInleitung und auch noch einen Charming Python-Artikel.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
BlackJack

Ich habe mich mal wieder an einer "funktionalen" Lösung versucht. Ungetestet:

Code: Alles auswählen

import os
import re
from itertools import imap

EMAIL_PATTERN = r'[-_a-z0-9.]+@[-.a-z0-9]+\.[a-z]{2,6}'
MAIL_PATH_PREFIX = os.path.join('/', 'var', 'vpopmail', 'domains')


def any(iterable):
    return True in imap(bool, iterable)


def iter_adresses(recipient):
    email_re = re.compile(EMAIL_PATTERN, re.IGNORECASE)
    return (match.group(0).split('@')
            for match in imap(email_re.search, recipient.split(','))
            if match)


def has_maildir((localpart, domain)):
    return os.path.isdir(os.path.join(MAIL_PATH_PREFIX, domain, localpart,
                                      'Maildir'))


def is_forward_address((localpart, domain)):
    address_re = re.compile('^&?%s$' % EMAIL_PATTERN, re.IGNORECASE)
    alias_path = os.path.join(MAIL_PATH_PREFIX, domain, '.qmail-' + localpart)
    result = False
    try:
        aliases = open(alias_path)
        for line in aliases:
            line = line.strip()
            if address_re.match(line) and is_valid_recipient(line):
                result = True
                break
            else: 
                result = True
                break
        aliases.close()
    except IOError:
        pass
    
    return result


def is_alias((localpart, domain)):
    return (localpart.startswith(domain + '-')
            and is_valid_recipient('%s@%s' % (localpart[len(domain + 1):],
                                              domain)))


def is_valid_address(address):
    return any(test(address) for test in (has_maildir,
                                          is_forward_address,
                                          is_alias))


def is_valid_recipient(recipient):
    return any(imap(is_valid_address, iter_adresses(recipient)))
Irgendwie glaube ich aber nicht das `is_forward_address()` so gemeint war und dementsprechend auch das Original fehlerhaft ist. Oder?
lunar

Sollte re.compile nicht eher außerhalb der Funktionen stehen? Sonst kompiliert man doch den Ausdruck bei jedem Funktionsaufruf neu, oder?
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Wow,
ich hoffe, Du hast das aus Spaß gemacht, auf jeden Fall vielen Dank!

Da sind einige Kniffe drin, die mich wirklich weiterbringen, auch wenn ich gestehen muss teilweise leichte Verständnisprobleme zu haben, hauptsächlich, was die any-Funktion angeht.

Auf jeden Fall sehr schlank, wenn ich auch die Lesbarkeit nicht gerade optimal finde, was aber wohl hauptsächlich daran liegt, dass mir Konstrukte wie

Code: Alles auswählen

return True in imap(bool, iterable)
oder

Code: Alles auswählen

return any(test(address) for test in (has_maildir,
                                          is_forward_address,
                                          is_alias))
völlig neu sind. Das muss ich mir erstmal auf der Zunge zergehen lassen.

Die printf-artige Syntax per % kannte ich auch noch nicht, feine Sache das.


Eine doofe Frage habe ich aber noch: Was macht der Doppelpunkt in Zeile 48 bei

Code: Alles auswählen

localpart[len(domain + 1):]
Und müsste das nicht

Code: Alles auswählen

localpart[len(domain) + 1:]
heißen?


Achso, das mit is_forward_address() stimmt soweit schon. Es gibt eine Textdatei, die .qmail-$accountname heißt und zeilenweise Forwards enthält. Das können E-Mail-Addressen (evtl. mit einem & davor) sein oder z.B. Programmeinträge etc.
Evtl. kommen noch andere Checks dazu, aber im Moment ist das ausreichend.



Ich möchte die Gelegenheit nutzen, um mal ein großes Lob an dieses Forum (besser gesagt dessen Benutzer) auszusprechen. Ich bin erst kurz hier, finde es aber grandios, wie viel Engagement ihr zeigt.
Man könnte fast meinen, ihr hättet Spaß an Python ;)
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Der Doppelpunkt steht für die Slice-Syntax: slicebares_objekt[startpunkt:endpunkt:schrittweite]. Wenn man Werte auslässt werden Defaultwerte benutzt, d.h. Startpunkt liegt am Anfang, Endpunkt am Ende und die Schrittweite beträgt 1. Man kann ebenso auch negative Werte benutzen, zum Beispiel slicebares_objekt[:-1] welches ein Slice von Anfang bis zum vorletzten Element zurückgibt.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
BlackJack

dst hat geschrieben:Da sind einige Kniffe drin, die mich wirklich weiterbringen, auch wenn ich gestehen muss teilweise leichte Verständnisprobleme zu haben, hauptsächlich, was die any-Funktion angeht.
`any()` gibt's ab Python 2.5 als eingebaute Funtkion, dann braucht man sich die nicht mehr selber definieren. :-)

Die Funktion bekommt ein "iterable" und gibt `True` zurück falls irgendein Element davon "Wahr" ist. Sobald so ein Element gefunden ist, werden die folgenden Elemente nicht mehr getestet, das bricht also ab sobald das Ergebnis feststeht, genau wie ``a or b or c or ...`` auch nur soweit ausgewertet wird, bis das Ergebnis sich nicht mehr ändern kann

Code: Alles auswählen

return True in imap(bool, iterable)
`imap()` bekommt eine Funktion und ein "iterable" und liefert ein "iterable" über die Elemente des zweiten Arguments, wobei auf jedes Element die Funktion angewendet wurde. In diesem Fall erhält man also für jedes ursprüngliche Element `True` oder `False`, je nachdem was ``bool(element)`` ergibt. Und mit dem ``in``-Operator wird geprüft ob `True` vorkommt, also ob irgendein Element wahr ist.

Code: Alles auswählen

    return any(test(address) for test in (has_maildir,
                                          is_forward_address,
                                          is_alias))
Hier werden die drei Testfunktionen nacheinander auf `address` angewendet und mit `any()` geprüft, ob eine davon `True` ergibt. Das in den Klammern von `any()` ist eine "generator expression", was dazu führt, dass dieser Test "lazy" ausgeführt wird weil `any()` die Ergebnisse nur solange konsumiert bis `True` auftaucht. Wenn also der erste Test schon erfolgreich ist, werden die restlichen nicht mehr ausgeführt.
Und müsste das nicht

Code: Alles auswählen

localpart[len(domain) + 1:]
heißen?
Ja. :oops:
Achso, das mit is_forward_address() stimmt soweit schon.
Bist Du Dir da sicher? Schau Dir die Logik nochmal genau an. Wenn es die Datei gibt, wird eigentlich grundsätzlich `True` zurückgegeben, ob nun eine passende Zeile in der Datei steht oder nicht. Das kann man dann auch kürzer schreiben indem man einfach nur prüft, ob die Datei vorhanden ist.
Man könnte fast meinen, ihr hättet Spaß an Python ;)
Üüüüüberhaupt nicht. Ich hasse Python. Und bin Masochist. ;-)
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Das muss ich mir erstmal alles in Ruhe durch den Kopf gehen lassen.

is_forward_address() soll nur False zurückgeben, wenn in der Datei .qmail-$localpart kein gültiges lokales Postfach, keine Weiterleitung auf ein lokales Postfach (daher die Rekursion) und kein Programmaufruf drinsteht.
Da alles was keine E-Mail-Adresse ist, ein Programmaufruf oder eine lokale Mailbox sein dürfte, wird tatsächlich in den meisten Fällen True zurückgegeben.
Einfacher ausgedrückt, nur wenn die Datei ausschließlich externe Adressen enthält, soll False zurückgegeben werden. Soweit ich das sehe, sollte mit deiner Version funktionieren.
is_forward_address() ist dabei nicht der perfekte Name, aber das ist eher nebensächlich ;)

Ich werd jetzt erstmal ein bisschen mit Pydev für Eclipse rumspielen, vim ist, finde ich, nicht unbedingt der beste Editor, zum lernen einer Sprache ;)


P.S. Bei der imap()-Funktion habe ich mich etwas foppen lassen, irgendwie dachte ich da an das Protokoll und nicht an eine Iteratorfunktion ;)

P.P.S.
Üüüüüberhaupt nicht. Ich hasse Python. Und bin Masochist. ;-)
Ich habe es befürchtet ;)
Leonidas
Python-Forum Veteran
Beiträge: 16025
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

dst hat geschrieben:P.S. Bei der imap()-Funktion habe ich mich etwas foppen lassen, irgendwie dachte ich da an das Protokoll und nicht an eine Iteratorfunktion ;)
Ja, der Name ist etwas verwirrend, aber ``imap`` ist einfach die lazy-Version von ``map`` aus den Builtins und die anderen Funktionen in den Itertools die Generatoren neu implementieren fangen auch mit ``i`` an, damit man sie eben von den Builtins unterscheiden kann und damit sie beim Import keine Builtins überschreiben.
My god, it's full of CARs! | Leonidasvoice vs (former) Modvoice
Antworten