@DGUV-V3: In wiefern ist der Inhalt nicht identisch? Was hast Du im Original, was erwartest Du, und was bekommst Du stattdessen?
Anmerkungen zum Quelltext: Der Kommentarblock am Anfang würde als Docstring für das Modul Sinn machen.
`os` wird importiert, aber nicht verwenden. Es sollte allerdings verwendet werden, denn Pfadteile setzt man mit `os.path.join()` statt mit Zeichenkettenoperationen zusammen.
Auf Modulebene werden nur Konstanten, Funktionen, und Klassen definiert. Das Hauptprogramm steht üblicherweise in einer Funktion die `main()` heisst, und die so ”geschützt” wird, das sie nur automatisch ausgeführt wird, wenn man das Modul als Programm ausführt und nicht wenn man es importiert. Dann kann man die Bestandteile einfacher testen und wiederverwenden.
Die Ausnahmebehandlung mit dem ”nackten” ``except:`` ohne konkrete Ausnahme(n) ist schlecht. Das behandelt *jede* Ausnahme, auch solche mit denen Du gar nicht rechnest. Und dem Benutzer wird gesagt der E-Mail-Anbieter wurde nicht gefunden. Auch wenn das Problem ein völlig anderes ist. Da Du das Programm sowieso abbrichst, kann man sich diese Art der Nichtbehandlung auch sparen und man bekommt automatisch bessere Informationen was tatsächlich das Problem ist.
Man könnte stattdessen mit einem ``finally`` dafür sorgen, dass am Ende die `quit()`-Methode aufgerufen wird, auch wenn vorher eine Ausnahme auftritt.
Ich würde die Rückgabe von `POP3.list()` nicht verwerfen und dann die Nachrichtennummern selber erzeugen. Du weisst doch gar nicht sicher ob die wirklich von 1 an aufsteigend und lückenlos sind. Das mag bei dem Server den Du getestet hast, bisher zufällig immer so gewesen sein, aber wenn das immer so wäre, dann würde die API von `POP3.list()` ja keinen Sinn machen!
`string.join()` ist veraltet und sollte, wie alle Funktionen aus dem `string`-Modul die es mittlerweile schon sehr lange als Methoden auf `str` gibt, nicht mehr verwendet werden.
Die Schreibweise von `saveAttachment()` entspricht nicht dem
Style Guide for Python Code und ist ausserdem inhaltlich nicht ganz korrekt, denn es wird nicht *eine* Anlage gespeichert, sondern *alle*. Es sollte also `save_attachments()` heissen.
Namen sollten nicht kryptisch abgekürzt, oder mit Prä- oder Suffixen versehen werden, sondern klar vermitteln was der Wert dahinter im Kontext des Programms bedeutet. An Wortgrenzen innerhalb eines namens macht ein Unterstrich das lesen/erfassen deutlich einfacher.
Anstatt paarweise zusammengehörende Daten in zwei ”paralellen” Listen zu speichern und am Ende über den Umweg eines Laufindexes wieder zusammen zu führen, speichert man die zusammengehörenden Daten besser *zusammen* in *einer* Liste. Zum Beispiel in dem man sie zu Tupeln zusammenfasst.
Ich bin schon ziemlich lange bei Python dabei, aber den ``<>``-Operator habe ich vielleicht zwei oder drei mal gesehen. Der ist veraltet und wird als ``!=`` geschrieben. Da leere Dateinamen nicht erlaubt sind, braucht man hier aber eigentlich auch gar keinen expliziten Test, sondern kann einfach den Dateinamen selbst als Wahrheitswert verwenden.
`file` war nicht zum öffnen von Dateien vorgesehen sondern als Datentyp um eigene Untertypen vom Dateityp abzuleiten. Dateien werden mit `open()` geöffnet. Und falls möglich zusammen mit der ``with``-Anweisung um den Code robuster und sicherer zu machen was Probleme zwischen dem öffnen und dem schliessen der Datei betrifft, die sonst dazu führen könnten, das die Datei nicht geschlossen wird.
Zeichenketten und Werte mit ``+`` zusammen zu stückeln ist eher BASIC als Python. Python kennt Zeichenkettenformatierung.
Ich komme dann ungefähr bei so etwas heraus:
Code: Alles auswählen
#!/usr/bin/env python
"""
attsave.py
Check emails at :const:`PROVIDER` for attachments and save them to
:const:`SAVEDIR`.
"""
from __future__ import absolute_import, division, print_function
import email
import os
import poplib
PROVIDER = 'pop3.web.de'
USER = 'xy@web.de'
PASSWORD = 'xyz'
SAVE_DIR = '/home/pi/EmailAnhang'
def save_attachments(mail_string):
attachments = list()
for part in email.message_from_string(mail_string).walk():
filename = part.get_filename()
if filename:
attachments.append((filename, part.get_payload()))
for filename, content in attachments:
with open(os.path.join(SAVE_DIR, filename), 'wb') as attachment_file:
attachment_file.write(content)
print('Found and saved attachment {0!r}.'.format(filename))
def main():
try:
client = poplib.POP3_SSL(PROVIDER)
client.user(USER)
client.pass_(PASSWORD)
for message_number in client.list()[1]:
save_attachments('\n'.join(client.retr(message_number)[1]))
finally:
client.quit()
if __name__ == '__main__':
main()
Der Umweg über das Dateisystem sollte allerdings am Ende nicht notwendig sein, und so wie das jetzt programmiert ist, ist es ausserdem noch sehr unsicher, denn man sollte den tatsächlichen Zielpfad überprüfen ob der auch wirklich innerhalb des gewünschten Zielverzeichnisses liegt. Sonst kann ein Angreifer nämlich E-Mails so präparieren dass er Dir gezielt Dateien überschreibt und sich so Zugang zu Deinem Rechner verschaffen kann.
