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.
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Bewertung meines ersten ernst gemeinten Python-Codes

Beitragvon dst » Freitag 2. März 2007, 05:05

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

Beitragvon BlackJack » Freitag 2. März 2007, 08:47

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

Beitragvon Luzandro » Freitag 2. März 2007, 12:43

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)
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Beitragvon dst » Freitag 2. März 2007, 15:50

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]
Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Freitag 2. März 2007, 16:33

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 Modvoice
Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Freitag 2. März 2007, 16:36

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 Modvoice
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Beitragvon dst » Freitag 2. März 2007, 17:04

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

Beitragvon Luzandro » Freitag 2. März 2007, 17:21

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
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Beitragvon dst » Freitag 2. März 2007, 17: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.
Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Freitag 2. März 2007, 19:03

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 Modvoice
dst
User
Beiträge: 27
Registriert: Mittwoch 28. Februar 2007, 23:42

Beitragvon dst » Freitag 2. März 2007, 21:48

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.
Benutzeravatar
Leonidas
Administrator
Beiträge: 16023
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Beitragvon Leonidas » Freitag 2. März 2007, 23:15

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 Modvoice
BlackJack

Beitragvon BlackJack » Samstag 3. März 2007, 16:20

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

Beitragvon lunar » Samstag 3. März 2007, 17:04

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

Beitragvon dst » Samstag 3. März 2007, 17:10

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 ;)

Wer ist online?

Mitglieder in diesem Forum: 0 Mitglieder