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

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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

+class EmailMessage:
+    """An email message.
+

^- Unless there is a specific reason not to, you should always use 'new-style'
classes, and inherit from 'object'.

class EmailMessage(object):

Some of the python stdlib doesn't, but mostly just because the code there is
older than new-style classes.


+    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
+        of __getitem__ in email.Message.
+        """

^- This method intentionally does not raise ...


+    def test_headers_accept_unicode_and_utf8(self):
+        for string 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

^- I would avoid using variables with the same name as obvious modules/builtin
objects. So:

 for username in []

is probably better.




+    def test_send(self):
+        class FakeConfig:
+            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
+
+        def verify_message_without_attachment(_self, msg):
+            self.assertEqualDiff(SIMPLE_MESSAGE_ASCII , msg.as_string())
+
+        old_send_email = SMTPConnection.send_email
+        try:
+            SMTPConnection.send_email = verify_message_with_mime_type()
+            EmailMessage.send(FakeConfig(), 'from at from.com', 'to at to.com',
+                    'subject', 'body', u'a\nb\nc\nd\ne\n', 'lines.txt')
+
+            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')
+
+            SMTPConnection.send_email = verify_message_without_attachment
+            EmailMessage.send(FakeConfig(), 'from at from.com', 'to at to.com',
+                    'subject', 'body')
+        finally:
+            SMTPConnection.send_email = old_send_email
+

You noticed in the past that the test passed when you didn't override
SMTPConnection.send_email, it just happened to actually send an email.

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.

For example:

messages = []
def send_as_append(_self, msg):
    messages.append(msg.as_string(BOUNDARY))

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[:] = [] # Clear the messages list

...

That way the test will fail if you fail to override send_email, rather than
accidentally sending an email.


=== modified file bzrlib/merge_directive.py
- --- bzrlib/merge_directive.py
+++ bzrlib/merge_directive.py
@@ -15,7 +15,6 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA


- -from email import Message
 from StringIO import StringIO
 import re

@@ -33,6 +32,7 @@
 from bzrlib.bundle import (
     serializer as bundle_serializer,
     )
+from bzrlib.email_message import EmailMessage


 class _BaseMergeDirective(object):
@@ -170,19 +170,16 @@
         :return: an email message
         """
         mail_from = branch.get_config().username()
- -        message = Message.Message()
- -        message['To'] = mail_to
- -        message['From'] = mail_from
         if self.message is not None:
- -            message['Subject'] = self.message
+            subject = self.message
         else:
             revision = branch.repository.get_revision(self.revision_id)
- -            message['Subject'] = revision.message
+            subject = revision.message
         if sign:
             body = self.to_signed(branch)
         else:
             body = ''.join(self.to_lines())
- -        message.set_payload(body)
+        message = EmailMessage(mail_from, mail_to, subject, body)
         return message


^- I personally think we should be sending merge directives as attachments, but
I wouldn't strictly require you to change that here. (For one thing, we don't
have a body to set at this point).

Though maybe subject should be the first line of 'message', and then all of
message should be the main body.

Which means you could send a merge directive with a full comment using:

bzr merge-directive --send-to ...  --message "Subject line
This is a comment about this patch
and all the other information
that I care to tell you.
"


The last comment is more for discussion on list.

All my other comments could be fixed up by the person doing the merge.

So +1 (conditional).

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGn7/8JdeBCYSNAAMRAtWHAKCk9NPeQx4e29VWbzPvWEGdnbxBTgCdF3uC
KfWJG4BZcSluyu1VjvClURM=
=NYt7
-----END PGP SIGNATURE-----