[MERGE] EmailMessage, the façade version (v3)

Adeodato Simó dato at net.com.org.es
Thu Jul 19 22:31:50 BST 2007


* John Arbash Meinel [Thu, 19 Jul 2007 14:48:13 -0500]:

> Adeodato Simó wrote:
> > Merging against latest bzr.dev results in conflicts in
> > test_merge_directive.py, so here is an updated version.

Hello again. Thanks for the review.

> +        This method does intentionally not raise KeyError to mimic the behavior
> ^- This method intentionally does not raise ...

Heh, sounded as proper English to me. O:-)


> +        for string in [ u'Pepe P\xe9rez <pperez at ejemplo.com>',
> ^- I would avoid using variables with the same name as obvious modules/builtin
> objects. So:

I changed that there, and elsewhere in the code where string was being
used as a variable.


> Because of this, I think it is better to not have the 'self.assert*' inside the
> overriding function, but instead have it update a local variable, and then the
> 'self.assert*' checks that.

Did that, thanks. Seems indeed useful to make sure the assert calls will
always be made.

Partial diff below.

> ^- I personally think we should be sending merge directives as attachments, but
> I wouldn't strictly require you to change that here.

Well, with submit/bundle now emiting merge directives, and the
prospective send command, seems like the merge-directive command would
be going away instead? Any point in fixing it?

Cheers,

=== modified file 'bzrlib/email_message.py'
--- bzrlib/email_message.py	2007-07-18 15:51:52 +0000
+++ bzrlib/email_message.py	2007-07-19 21:22:53 +0000
@@ -29,7 +29,7 @@
 from bzrlib.smtp_connection import SMTPConnection
 
 
-class EmailMessage:
+class EmailMessage(object):
     """An email message.
     
     The constructor needs an origin address, a destination address or addresses
@@ -107,8 +107,8 @@
         if not self._parts:
             self._msgobj = Message.Message()
             if self._body is not None:
-                string, encoding = self.string_with_encoding(self._body)
-                self._msgobj.set_payload(string, encoding)
+                body, encoding = self.string_with_encoding(self._body)
+                self._msgobj.set_payload(body, encoding)
         else:
             self._msgobj = MIMEMultipart.MIMEMultipart()
 
@@ -116,8 +116,8 @@
                 self._msgobj.set_boundary(boundary)
 
             for body, filename, mime_subtype in self._parts:
-                string, encoding = self.string_with_encoding(body)
-                payload = MIMEText.MIMEText(string, mime_subtype, encoding)
+                body, encoding = self.string_with_encoding(body)
+                payload = MIMEText.MIMEText(body, mime_subtype, encoding)
 
                 if filename is not None:
                     content_type = payload['Content-Type']
@@ -142,7 +142,7 @@
     def __getitem__(self, header):
         """Get a header from the message, returning None if not present.
         
-        This method does intentionally not raise KeyError to mimic the behavior
+        This method intentionally does not raise KeyError to mimic the behavior
         of __getitem__ in email.Message.
         """
         return self._headers.get(header, None)
@@ -183,10 +183,10 @@
                 email))
 
     @staticmethod
-    def string_with_encoding(string):
+    def string_with_encoding(string_):
         """Return a str object together with an encoding.
 
-        :param string: A str or unicode object.
+        :param string_: A str or unicode object.
         :return: A tuple (str, encoding), where encoding is one of 'ascii',
             'utf-8', or '8-bit', in that preferred order.
         """
@@ -195,18 +195,18 @@
         # avoid base64 when it's not necessary in order to be most compatible
         # with the capabilities of the receiving side, we check with encode()
         # and decode() whether the body is actually ascii-only.
-        if isinstance(string, unicode):
+        if isinstance(string_, unicode):
             try:
-                return (string.encode('ascii'), 'ascii')
+                return (string_.encode('ascii'), 'ascii')
             except UnicodeEncodeError:
-                return (string.encode('utf-8'), 'utf-8')
+                return (string_.encode('utf-8'), 'utf-8')
         else:
             try:
-                string.decode('ascii')
-                return (string, 'ascii')
+                string_.decode('ascii')
+                return (string_, 'ascii')
             except UnicodeDecodeError:
                 try:
-                    string.decode('utf-8')
-                    return (string, 'utf-8')
+                    string_.decode('utf-8')
+                    return (string_, 'utf-8')
                 except UnicodeDecodeError:
-                    return (string, '8-bit')
+                    return (string_, '8-bit')

=== modified file 'bzrlib/tests/test_email_message.py'
--- bzrlib/tests/test_email_message.py	2007-07-18 15:51:52 +0000
+++ bzrlib/tests/test_email_message.py	2007-07-19 21:22:53 +0000
@@ -111,9 +111,9 @@
                 msg.as_string(BOUNDARY))
 
     def test_headers_accept_unicode_and_utf8(self):
-        for string in [ u'Pepe P\xe9rez <pperez at ejemplo.com>',
+        for user in [ u'Pepe P\xe9rez <pperez at ejemplo.com>',
                 'Pepe P\xc3\xa9red <pperez at ejemplo.com>' ]:
-            msg = EmailMessage(string, string, string) # no exception raised
+            msg = EmailMessage(user, user, user) # no exception raised
 
             for header in ['From', 'To', 'Subject']:
                 value = msg[header]
@@ -153,30 +153,32 @@
             def get_user_option(self, option):
                 return None
 
-        def verify_message_with_mime_type(mime_subtype='plain'):
-            def wrapper(_self, msg):
-                self.assertEqualDiff(COMPLEX_MULTIPART_MESSAGE % mime_subtype,
-                        msg.as_string(BOUNDARY))
-            return wrapper
+        messages = []
 
-        def verify_message_without_attachment(_self, msg):
-            self.assertEqualDiff(SIMPLE_MESSAGE_ASCII , msg.as_string())
+        def send_as_append(_self, msg):
+            messages.append(msg.as_string(BOUNDARY))
 
         old_send_email = SMTPConnection.send_email
         try:
-            SMTPConnection.send_email = verify_message_with_mime_type()
+            SMTPConnection.send_email = send_as_append
+
             EmailMessage.send(FakeConfig(), 'from at from.com', 'to at to.com',
                     'subject', 'body', u'a\nb\nc\nd\ne\n', 'lines.txt')
+            self.assertEqualDiff(COMPLEX_MULTIPART_MESSAGE % 'plain',
+                    messages[0])
+            messages[:] = []
 
-            SMTPConnection.send_email = verify_message_with_mime_type(
-                    'x-patch')
             EmailMessage.send(FakeConfig(), 'from at from.com', 'to at to.com',
                     'subject', 'body', u'a\nb\nc\nd\ne\n', 'lines.txt',
                     'x-patch')
+            self.assertEqualDiff(COMPLEX_MULTIPART_MESSAGE % 'x-patch',
+                    messages[0])
+            messages[:] = []
 
-            SMTPConnection.send_email = verify_message_without_attachment
             EmailMessage.send(FakeConfig(), 'from at from.com', 'to at to.com',
                     'subject', 'body')
+            self.assertEqualDiff(SIMPLE_MESSAGE_ASCII , messages[0])
+            messages[:] = []
         finally:
             SMTPConnection.send_email = old_send_email
 
@@ -220,5 +222,5 @@
                 'P\xc3\xa9rez': ('P\xc3\xa9rez', 'utf-8'),
                 'P\xe9rez':     ('P\xe9rez', '8-bit'),
         }
-        for string, pair in pairs.items():
-            self.assertEqual(pair, EmailMessage.string_with_encoding(string))
+        for string_, pair in pairs.items():
+            self.assertEqual(pair, EmailMessage.string_with_encoding(string_))


-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
If you want the holes in your knowledge showing up try teaching someone.
                -- Alan Cox




More information about the bazaar mailing list