Рефакторинг входящего poplib с использованием windows python 2.3

Question

Привет, ребята, не могли бы вы помочь мне сделать рефакторинг, чтобы он был явно питоническим.

import sys
import poplib
import string
import StringIO, rfc822
import datetime
import logging

def _dump_pop_emails(self):
    self.logger.info("open pop account %s with username: %s" % (self.account[0], self.account[1]))
    self.popinstance = poplib.POP3(self.account[0])
    self.logger.info(self.popinstance.getwelcome()) 
    self.popinstance.user(self.account[1])
    self.popinstance.pass_(self.account[2])
    try:
        (numMsgs, totalSize) = self.popinstance.stat()
        for thisNum in range(1, numMsgs+1):
            (server_msg, body, octets) = self.popinstance.retr(thisNum)
            text = string.join(body, '\n')
            mesg = StringIO.StringIO(text)                               
            msg = rfc822.Message(mesg)
            name, email = msg.getaddr("From")
            emailpath = str(self._emailpath + self._inboxfolder + "\\" + email + "_" + msg.getheader("Subject") + ".eml")
            emailpath = self._replace_whitespace(emailpath)
            file = open(emailpath,"wb")
            file.write(text)
            file.close()
            self.popinstance.dele(thisNum)
    finally:
        self.logger.info(self.popinstance.quit())

def _replace_whitespace(self,name):
    name = str(name)
    return name.replace(" ", "_")   

Также в методе _replace_whitespace я хотел бы иметь некоторую процедуру очистки, которая удаляет все недопустимые символы, которые могут вызвать обработку.

В основном я хочу написать письмо в папку входящих сообщений стандартным способом.

Я делаю что-то здесь не так?

Answer 1

Это не рефакторинг (насколько я вижу, он не требует рефакторинга), но некоторые предложения:

Вы должны использовать пакет электронной почты, а не rfc822. Замените rfc822.Message на email.Message и используйте email.Utils.parseaddr(msg["From"]), чтобы получить имя и адрес электронной почты, и msg["Subject"], чтобы получить тему.

Используйте os.path.join для создания пути. Это:

emailpath = str(self._emailpath + self._inboxfolder + "\\" + email + "_" + msg.getheader("Subject") + ".eml")

становится:

emailpath = os.path.join(self._emailpath + self._inboxfolder, email + "_" + msg.getheader("Subject") + ".eml")

(Если self._inboxfolder начинается с косой черты или self._emailpath заканчивается на один, вы можете также заменить первый + запятой).

На самом деле это ничего не мешает, но вам, вероятно, не следует использовать "file" в качестве имени переменной, поскольку он скрывает встроенный тип (шашки, такие как pylint или pychecker, предупредят вас об этом).

Если вы не используете self.popinstance вне этой функции (кажется маловероятным, учитывая, что вы подключаетесь и выходите из функции), то нет смысла делать его атрибутом self. Просто используйте "popinstance" сам по себе.

Используйте xrange вместо range.

Вместо того, чтобы просто импортировать StringIO, сделайте это:

try:
    import cStringIO as StringIO
except ImportError:
    import StringIO

Если это почтовый ящик POP, к которому одновременно могут обращаться несколько клиентов, вам может потребоваться выполнить попытку / исключение вокруг вызова RETR, чтобы продолжить, если вы не можете получить одно сообщение.

Как сказал Джон, используйте "\n".join вместо string.join, используйте try/finally, чтобы закрыть файл, только если он открыт, и отдельно передайте параметры ведения журнала.

Единственная проблема с рефакторингом, о которой я мог подумать, заключается в том, что вам на самом деле не нужно анализировать все сообщение, поскольку вы просто выгружаете копию необработанных байтов, и все, что вам нужно, это заголовки From и Subject. Вместо этого вы можете использовать popinstance.top(0), чтобы получить заголовки, создать из него сообщение (пустое тело) и использовать его для заголовков. Затем выполните полное RETR, чтобы получить байты. Это стоило бы сделать только в том случае, если ваши сообщения были большими (поэтому их анализ занимал много времени). Я определенно измерил бы, прежде чем я сделал эту оптимизацию.

От того, как ваша функция очищает имена, зависит, насколько хороши вы хотите, чтобы имена были, и насколько вы уверены, что электронная почта и тема делают имя файла уникальным (кажется довольно маловероятным). Вы могли бы сделать что-то вроде:

emailpath = "".join([c for c in emailpath if c in (string.letters + string.digits + "_ ")])

И вы получите только буквенно-цифровые символы, знак подчеркивания и пробел, которые кажутся читаемыми. Учитывая, что ваша файловая система (с Windows), вероятно, нечувствительна к регистру, вы также можете сделать это в нижнем регистре (добавьте.lower() до конца). Вы можете использовать emailpath.translate, если хотите что-то более сложное.

Answer 2

Я не вижу в этом коде ничего существенного неправильного - он ведет себя неправильно или вы просто ищете общие рекомендации по стилю?

Несколько заметок:

1. Вместо logger.info ("foo %s %s" % (bar, baz))использовать "foo %s %s", bar, baz, Это позволяет избежать накладных расходов на форматирование строки, если сообщение не будет напечатано.
2. Положить try...finally вокруг открытия emailpath,
3. использование '\n'.join (body), вместо string.join (body, '\n'),
4. Вместо msg.getaddr("From"), просто msg.From,

Answer 3

В дополнение к моему комментарию на ответ Джона

Я выяснил, в чем проблема: в поле имени и в поле "Тема" были недопустимые символы, из-за чего Python пытался записать сообщение в виде каталога, увидев ":" и "/".

Джон Точка № 4 не работает! поэтому я оставил это как прежде. Также верно ли пункт № 1, правильно ли я выполнил ваше предложение?

def _dump_pop_emails(self):
    self.logger.info("open pop account %s with username: %s", self.account[0], self.account[1])
    self.popinstance = poplib.POP3(self.account[0])
    self.logger.info(self.popinstance.getwelcome()) 
    self.popinstance.user(self.account[1])
    self.popinstance.pass_(self.account[2])
    try:
        (numMsgs, totalSize) = self.popinstance.stat()
        for thisNum in range(1, numMsgs+1):
            (server_msg, body, octets) = self.popinstance.retr(thisNum)
            text = '\n'.join(body)
            mesg = StringIO.StringIO(text)                               
            msg = rfc822.Message(mesg)
            name, email = msg.getaddr("From")
            emailpath = str(self._emailpath + self._inboxfolder + "\\" + self._sanitize_string(email + " " + msg.getheader("Subject") + ".eml"))
            emailpath = self._replace_whitespace(emailpath)
            print emailpath
            file = open(emailpath,"wb")
            file.write(text)
            file.close()
            self.popinstance.dele(thisNum)
    finally:
        self.logger.info(self.popinstance.quit())

def _replace_whitespace(self,name):
    name = str(name)
    return name.replace(" ", "_")   

def _sanitize_string(self,name):
    illegal_chars = ":", "/", "\\"
    name = str(name)
    for item in illegal_chars:
        name = name.replace(item, "_")
    return name