launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-mailbox into launchpad:master.

Commit message:
Port MboxMailer to the modern mailbox API

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/386530

This also does a better job of producing valid mbox files; we were previously missing the "From " line at the start of each message.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-mailbox into launchpad:master.

diff --git a/lib/lp/services/mail/mbox.py b/lib/lp/services/mail/mbox.py
index 1767523..8cb2942 100644
--- a/lib/lp/services/mail/mbox.py
+++ b/lib/lp/services/mail/mbox.py
@@ -3,13 +3,15 @@
 
 """An IMailer that stores messages in a specified mbox file."""
 
+from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 
-
+from contextlib import closing
 import email
 from email.utils import make_msgid
 from logging import getLogger
+import mailbox
 
 from zope.component import getUtility
 from zope.interface import implementer
@@ -21,10 +23,7 @@ COMMASPACE = ', '
 
 @implementer(IMailer)
 class MboxMailer:
-    """
-    Stores the message in a Unix mailbox file.  This will be so much cooler
-    when we can use Python 2.5's mailbox module.
-    """
+    """Stores the message in a Unix mailbox file."""
 
     def __init__(self, filename, overwrite, mailer=None):
         self.filename = filename
@@ -33,8 +32,8 @@ class MboxMailer:
             # this is effectively an overwrite.  Note that because IMailer
             # doesn't have a close() method, we can't leave the file open
             # here, otherwise it will never get closed.
-            mbox_file = open(self.filename, 'w')
-            mbox_file.close()
+            with open(self.filename, 'w'):
+                pass
         self.mailer = mailer
 
     def send(self, fromaddr, toaddrs, message):
@@ -56,11 +55,12 @@ class MboxMailer:
         # zap it first just in case.
         del msg['message-id']
         msg['Message-ID'] = message_id = make_msgid()
-        mbox_file = open(self.filename, 'a')
-        try:
-            print >> mbox_file, msg
-        finally:
-            mbox_file.close()
+        with closing(mailbox.mbox(self.filename)) as mbox:
+            mbox.lock()
+            try:
+                mbox.add(msg)
+            finally:
+                mbox.unlock()
         if self.mailer is not None:
             # Forward the message on to the chained mailer, if there is one.
             # This allows for example, the mboxMailer to be used in the test
diff --git a/lib/lp/services/mail/tests/mbox_mailer.txt b/lib/lp/services/mail/tests/mbox_mailer.txt
index 8e88bd2..3495dc5 100644
--- a/lib/lp/services/mail/tests/mbox_mailer.txt
+++ b/lib/lp/services/mail/tests/mbox_mailer.txt
@@ -20,12 +20,12 @@ the temporary file mbox_filename for us.
     >>> msg_id
     '<...>'
 
-Read the mbox file and make sure the message we just mailed is in there.
+Read the mbox file and make sure the message we just mailed is in there, and
+no other messages.
 
-    >>> from mailbox import UnixMailbox
-    >>> mbox_file = open(mbox_filename)
-    >>> mbox = UnixMailbox(mbox_file)
-    >>> msg = next(mbox)
+    >>> import mailbox
+    >>> mbox = mailbox.mbox(mbox_filename)
+    >>> [msg] = mbox
     >>> msg['from']
     'geddy@xxxxxxxxxxx'
     >>> msg['to']
@@ -40,11 +40,7 @@ Read the mbox file and make sure the message we just mailed is in there.
     'jaco@xxxxxxxxxxx, victor@xxxxxxxxxxx'
     >>> msg['message-id'] == msg_id
     True
-
-There should be no other messages in the mbox file.
-
-    >>> next(mbox)
-    >>> mbox_file.close()
+    >>> mbox.close()
 
 Create another mailer, again that overwrites.  Make sure it actually does
 overwrite.
@@ -61,13 +57,11 @@ overwrite.
     ... There is a bug we should be concerned with.""")
     '<...>'
 
-    >>> mbox_file = open(mbox_filename)
-    >>> mbox = UnixMailbox(mbox_file)
-    >>> msg = next(mbox)
+    >>> mbox = mailbox.mbox(mbox_filename)
+    >>> [msg] = mbox
     >>> msg['from']
     'mick@xxxxxxxxxxx'
-    >>> next(mbox)
-    >>> mbox_file.close()
+    >>> mbox.close()
 
 Create another mailer, this time one that does not overwrite.  Both the
 message we sent above and the message we're about to send should be in the
@@ -85,15 +79,13 @@ mbox file.
     ... There is a bug we should be concerned with.""")
     '<...>'
 
-    >>> mbox_file = open(mbox_filename)
-    >>> mbox = UnixMailbox(mbox_file)
-    >>> msg = next(mbox)
-    >>> msg['from']
+    >>> mbox = mailbox.mbox(mbox_filename)
+    >>> [msg1, msg2] = mbox
+    >>> msg1['from']
     'mick@xxxxxxxxxxx'
-    >>> msg = next(mbox)
-    >>> msg['from']
+    >>> msg2['from']
     'carol@xxxxxxxxxxx'
-    >>> mbox_file.close()
+    >>> mbox.close()
 
 Now test mailer chaining.  Because we don't want these tests to depend on any
 other kind of mailer, create two mbox mailers, chaining one to the other.  The
@@ -118,16 +110,14 @@ harness.
     ... There is a bug we should be concerned with.""")
     '<...>'
 
-    >>> mbox_file = open(mbox_filename)
-    >>> mbox = UnixMailbox(mbox_file)
-    >>> msg = next(mbox)
+    >>> mbox = mailbox.mbox(mbox_filename)
+    >>> [msg] = mbox
     >>> msg['from']
     'sting@xxxxxxxxxxx'
-    >>> msg = next(mbox)
+    >>> mbox.close()
 
-    >>> mbox_file = open(chained_filename)
-    >>> mbox = UnixMailbox(mbox_file)
-    >>> msg = next(mbox)
+    >>> mbox = mailbox.mbox(chained_filename)
+    >>> [msg] = mbox
     >>> msg['from']
     'sting@xxxxxxxxxxx'
-    >>> msg = next(mbox)
+    >>> mbox.close()