Repository: awdeorio/mailmerge
Pull request #62 (2019-12-14 16:02:06)

Pathlib
Use pathlib's `Path` objects instead of strings in both implementation code and unit tests.


Patch:
diff --git a/mailmerge/__main__.py b/mailmerge/__main__.py
index 9067b88..8d451c4 100644
--- a/mailmerge/__main__.py
+++ b/mailmerge/__main__.py
@@ -4,10 +4,7 @@ Command line interface implementation.
 Andrew DeOrio <awdeorio@umich.edu>
 """
 from __future__ import print_function
-import os
-import io
 import sys
-import csv
 import socket
 import configparser
 import smtplib
@@ -17,6 +14,18 @@ from .template_message import TemplateMessage
 from .sendmail_client import SendmailClient
 from .utils import MailmergeError
 
+# Python 2 pathlib support requires backport
+try:
+    from pathlib2 import Path
+except ImportError:
+    from pathlib import Path
+
+# Python 2 UTF8 support requires csv backport
+try:
+    from backports import csv
+except ImportError:
+    import csv
+
 
 @click.command(context_settings={"help_option_names": ['-h', '--help']})
 @click.version_option()  # Auto detect version from setup.py
@@ -61,6 +70,12 @@ def cli(sample, dry_run, limit, no_limit,
     # of local variables.
     # pylint: disable=too-many-arguments, too-many-locals
 
+    # Convert paths from string to Path objects
+    # https://github.com/pallets/click/issues/405
+    template_path = Path(template_path)
+    database_path = Path(database_path)
+    config_path = Path(config_path)
+
     check_input_files(template_path, database_path, config_path, sample)
 
     # No limit is an alias for limit=-1
@@ -124,11 +139,11 @@ def check_input_files(template_path, database_path, config_path, sample):
             config_path,
         )
         sys.exit(0)
-    if not os.path.exists(template_path):
+    if not template_path.exists():
         print("Error: can't find template email " + template_path)
         print("Create a sample (--sample) or specify a file (--template)")
         sys.exit(1)
-    if not os.path.exists(database_path):
+    if not database_path.exists():
         print("Error: can't find database_path " + database_path)
         print("Create a sample (--sample) or specify a file (--database)")
         sys.exit(1)
@@ -139,10 +154,10 @@ def create_sample_input_files(template_path,
                               config_path):
     """Create sample template email and database."""
     print("Creating sample template email {}".format(template_path))
-    if os.path.exists(template_path):
+    if template_path.exists():
         print("Error: file exists: " + template_path)
         sys.exit(1)
-    with io.open(template_path, "w") as template_file:
+    with template_path.open("w") as template_file:
         template_file.write(
             u"TO: {{email}}\n"
             u"SUBJECT: Testing mailmerge\n"
@@ -153,20 +168,20 @@ def create_sample_input_files(template_path,
             u"Your number is {{number}}.\n"
         )
     print("Creating sample database {}".format(database_path))
-    if os.path.exists(database_path):
+    if database_path.exists():
         print("Error: file exists: " + database_path)
         sys.exit(1)
-    with io.open(database_path, "w") as database_file:
+    with database_path.open("w") as database_file:
         database_file.write(
             u'email,name,number\n'
             u'myself@mydomain.com,"Myself",17\n'
             u'bob@bobdomain.com,"Bob",42\n'
         )
     print("Creating sample config file {}".format(config_path))
-    if os.path.exists(config_path):
+    if config_path.exists():
         print("Error: file exists: " + config_path)
         sys.exit(1)
-    with io.open(config_path, "w") as config_file:
+    with config_path.open("w") as config_file:
         config_file.write(
             u"# Example: GMail\n"
             u"[smtp_server]\n"
@@ -208,7 +223,7 @@ def create_sample_input_files(template_path,
 
 def read_csv_database(database_path):
     """Read database CSV file, providing one line at a time."""
-    with io.open(database_path, "r") as database_file:
+    with database_path.open("r") as database_file:
         reader = csv.DictReader(database_file)
         for row in reader:
             yield row
diff --git a/mailmerge/sendmail_client.py b/mailmerge/sendmail_client.py
index 2941b8d..f3b64ad 100644
--- a/mailmerge/sendmail_client.py
+++ b/mailmerge/sendmail_client.py
@@ -21,7 +21,7 @@ class SendmailClient(object):
     def __init__(self, config_filename, dry_run=False):
         """Read configuration from config_filename."""
         config = configparser.RawConfigParser()
-        config.read(config_filename)
+        config.read(str(config_filename))  # str need for older Python versions
         self.dry_run = dry_run
         self.host = config.get("smtp_server", "host")
         self.port = config.getint("smtp_server", "port")
diff --git a/mailmerge/template_message.py b/mailmerge/template_message.py
index d20815c..0366d6e 100644
--- a/mailmerge/template_message.py
+++ b/mailmerge/template_message.py
@@ -4,7 +4,6 @@ Represent a templated email message.
 Andrew DeOrio <awdeorio@umich.edu>
 """
 
-import os
 import future.backports.email as email
 import future.backports.email.mime
 import future.backports.email.mime.application
@@ -18,6 +17,12 @@ import jinja2
 import chardet
 from . import utils
 
+# Python 2 pathlib support requires backport
+try:
+    from pathlib2 import Path
+except ImportError:
+    from pathlib import Path
+
 
 class TemplateMessage(object):
     """Represent a templated email message.
@@ -36,18 +41,21 @@ class TemplateMessage(object):
 
     def __init__(self, template_path):
         """Initialize variables and Jinja2 template."""
-        self.template_path = template_path
+        self.template_path = Path(template_path)
         self._message = None
         self._sender = None
         self._recipients = None
 
-        # Configure Jinja2 template engine
+        # Configure Jinja2 template engine with the template dirname as root.
+        #
+        # Note: jinja2's FileSystemLoader does not support pathlib Path objects
+        # in Python 2. https://github.com/pallets/jinja/pull/1064
         template_env = jinja2.Environment(
-            loader=jinja2.FileSystemLoader(os.path.dirname(template_path)),
+            loader=jinja2.FileSystemLoader(str(template_path.parent)),
             undefined=jinja2.StrictUndefined,
         )
         self.template = template_env.get_template(
-            os.path.basename(template_path),
+            template_path.parts[-1],  # basename
         )
 
     def render(self, context):
@@ -120,38 +128,52 @@ class TemplateMessage(object):
 
     def _transform_attachments(self):
         """Parse Attachment headers and add attachments."""
+        # Do nothing if message has no attachment header
         if 'attachment' not in self._message:
             return
 
+        # Make sure the message is multipart.  We need a multipart message in
+        # order to add an attachment.
         self._make_message_multipart()
 
-        attachment_filepaths = self._message.get_all('attachment', failobj=[])
-        template_parent_dir = os.path.dirname(self.template_path)
-
-        for attachment_filepath in attachment_filepaths:
-            attachment_filepath = attachment_filepath.strip()
-            attachment_filepath = os.path.expanduser(attachment_filepath)
-            if not attachment_filepath:
-                continue
-            if not os.path.isabs(attachment_filepath):
-                # Relative paths are relative to the template's parent dir
-                attachment_filepath = os.path.join(template_parent_dir,
-                                                   attachment_filepath)
-            normalized_path = os.path.abspath(attachment_filepath)
-
-            # Check that the attachment exists
-            if not os.path.exists(normalized_path):
-                raise utils.MailmergeError(
-                    "Attachment not found: {}".format(normalized_path)
-                )
-            filename = os.path.basename(normalized_path)
-            with open(normalized_path, "rb") as attachment:
-                part = email.mime.application.MIMEApplication(
-                    attachment.read(),
-                    Name=filename,
-                )
-            part.add_header('Content-Disposition',
-                            'attachment; filename="{}"'.format(filename))
+        # Add each attachment to the message
+        for path in self._message.get_all('attachment', failobj=[]):
+            path = self._resolve_attachment_path(path)
+            with path.open("rb") as attachment:
+                content = attachment.read()
+            basename = path.parts[-1]
+            part = email.mime.application.MIMEApplication(
+                content,
+                Name=str(basename),
+            )
+            part.add_header(
+                'Content-Disposition',
+                'attachment; filename="{}"'.format(basename),
+            )
             self._message.attach(part)
 
+        # Remove the attachment header, it's non-standard for email
         del self._message['attachment']
+
+    def _resolve_attachment_path(self, path):
+        """Find attachment file or raise MailmergeError."""
+        # Error on empty path
+        if not path.strip():
+            raise utils.MailmergeError("Empty attachment header.")
+
+        # Create a Path object and handle home directory (tilde ~) notation
+        path = Path(path.strip())
+        path = path.expanduser()
+
+        # Relative paths are relative to the template's parent dir
+        if not path.is_absolute():
+            path = self.template_path.parent/path
+
+        # Resolve any symlinks
+        path = path.resolve()
+
+        # Check that the attachment exists
+        if not path.exists():
+            raise utils.MailmergeError("Attachment not found: {}".format(path))
+
+        return path
diff --git a/setup.py b/setup.py
index ff44307..2160d8e 100644
--- a/setup.py
+++ b/setup.py
@@ -21,19 +21,19 @@ setup(
     packages=["mailmerge"],
     keywords=["mail merge", "mailmerge", "email"],
     install_requires=[
-        "backports.csv;python_version<='2.7'",
+        "backports.csv;python_version<'3.0'",
         "chardet",
         "click",
         "configparser",
-        "jinja2",
 
         # The attachments feature relies on a bug fix in the future library
         # https://github.com/awdeorio/mailmerge/pull/56
         "future>0.18.0",
 
-        "backports.csv;python_version<='2.7'",
+        "jinja2",
         "markdown",
-        "mock;python_version<='2.7'",
+        "mock;python_version<'3.0'",
+        "pathlib2;python_version<'3.6'",
         "sh",
     ],
     extras_require={


Test patch:
diff --git a/tests/test_mailmerge.py b/tests/test_mailmerge.py
index a587b06..062d255 100644
--- a/tests/test_mailmerge.py
+++ b/tests/test_mailmerge.py
@@ -3,7 +3,6 @@ System tests.
 
 Andrew DeOrio <awdeorio@umich.edu>
 """
-import os
 import re
 import sh
 from . import utils
@@ -17,9 +16,9 @@ def test_stdout():
     """
     mailmerge_cmd = sh.Command("mailmerge")
     output = mailmerge_cmd(
-        "--template", os.path.join(utils.TESTDATA, "simple_template.txt"),
-        "--database", os.path.join(utils.TESTDATA, "simple_database.csv"),
-        "--config", os.path.join(utils.TESTDATA, "server_open.conf"),
+        "--template", utils.TESTDATA/"simple_template.txt",
+        "--database", utils.TESTDATA/"simple_database.csv",
+        "--config", utils.TESTDATA/"server_open.conf",
         "--no-limit",
         "--dry-run",
     )
diff --git a/tests/test_sendmail_client.py b/tests/test_sendmail_client.py
index 98f37b6..b523f15 100644
--- a/tests/test_sendmail_client.py
+++ b/tests/test_sendmail_client.py
@@ -3,7 +3,6 @@ Tests for SendmailClient.
 
 Andrew DeOrio <awdeorio@umich.edu>
 """
-import os
 import future.backports.email as email
 import future.backports.email.parser  # pylint: disable=unused-import
 from mailmerge.sendmail_client import SendmailClient
@@ -23,7 +22,7 @@ except ImportError:
 def test_smtp(mock_SMTP):
     """Verify SMTP library calls."""
     sendmail_client = SendmailClient(
-        os.path.join(utils.TESTDATA, "server_open.conf"),
+        utils.TESTDATA/"server_open.conf",
         dry_run=False,
     )
     message = email.parser.Parser().parsestr(u"""
diff --git a/tests/test_template_message.py b/tests/test_template_message.py
index eb359f5..1dc59f1 100644
--- a/tests/test_template_message.py
+++ b/tests/test_template_message.py
@@ -3,15 +3,13 @@ Tests for TemplateMessage.
 
 Andrew DeOrio <awdeorio@umich.edu>
 """
-import os
-import io
 import jinja2
 import pytest
 import markdown
 import mailmerge.template_message
 from . import utils
 
-# Python 2.x UTF8 support requires csv backport
+# Python 2 UTF8 support requires csv backport
 try:
     from backports import csv
 except ImportError:
@@ -21,7 +19,7 @@ except ImportError:
 def test_bad_jinja():
     """Bad jinja template should produce an error."""
     template_message = mailmerge.template_message.TemplateMessage(
-        os.path.join(utils.TESTDATA, "bad_template.txt"),
+        utils.TESTDATA/"bad_template.txt",
     )
     with pytest.raises(jinja2.exceptions.UndefinedError):
         template_message.render({"name": "Bob", "number": 17})
@@ -30,7 +28,7 @@ def test_bad_jinja():
 def test_cc_bcc():
     """CC recipients should receive a copy."""
     template_message = mailmerge.template_message.TemplateMessage(
-        template_path=os.path.join(utils.TESTDATA, "cc_bcc_template.txt"),
+        template_path=utils.TESTDATA/"cc_bcc_template.txt",
     )
     sender, recipients, message = template_message.render({
         "email": "myself@mydomain.com",
@@ -55,7 +53,7 @@ def test_cc_bcc():
 def test_markdown():
     """Markdown messages should be converted to HTML."""
     template_message = mailmerge.template_message.TemplateMessage(
-        template_path=os.path.join(utils.TESTDATA, "markdown_template.txt"),
+        template_path=utils.TESTDATA/"markdown_template.txt",
     )
     sender, recipients, message = template_message.render({
         "email": "myself@mydomain.com",
@@ -92,7 +90,7 @@ def test_markdown():
 def test_attachment():
     """Attachments should be sent as part of the email."""
     template_message = mailmerge.template_message.TemplateMessage(
-        template_path=os.path.join(utils.TESTDATA, "attachment_template.txt"),
+        template_path=utils.TESTDATA/"attachment_template.txt",
     )
     sender, recipients, message = template_message.render({
         "email": "myself@mydomain.com",
@@ -128,8 +126,7 @@ def test_attachment():
             file_contents = part.get_payload(decode=True)
             assert filename in expected_attachments
             assert not expected_attachments[filename]
-            filename_testdata = os.path.join(utils.TESTDATA, filename)
-            with open(filename_testdata, 'rb') as expected_attachment:
+            with (utils.TESTDATA/filename).open('rb') as expected_attachment:
                 correct_file_contents = expected_attachment.read()
             assert file_contents == correct_file_contents
             expected_attachments[filename] = True
@@ -137,10 +134,19 @@ def test_attachment():
     assert False not in expected_attachments.values()
 
 
+def test_attachment_empty():
+    """Errr on empty attachment field."""
+    template_message = mailmerge.template_message.TemplateMessage(
+        template_path=utils.TESTDATA/"attachment_template_empty.txt",
+    )
+    with pytest.raises(mailmerge.utils.MailmergeError):
+        template_message.render({})
+
+
 def test_utf8_template():
     """Verify UTF8 support in email template."""
     template_message = mailmerge.template_message.TemplateMessage(
-        template_path=os.path.join(utils.TESTDATA, "utf8_template.txt"),
+        template_path=utils.TESTDATA/"utf8_template.txt",
     )
     sender, recipients, message = template_message.render({
         "email": "myself@mydomain.com",
@@ -165,10 +171,9 @@ def test_utf8_template():
 def test_utf8_database():
     """Verify UTF8 support when template is rendered with UTF-8 value."""
     template_message = mailmerge.template_message.TemplateMessage(
-        template_path=os.path.join(utils.TESTDATA, "simple_template.txt"),
+        template_path=utils.TESTDATA/"simple_template.txt",
     )
-    database_path = os.path.join(utils.TESTDATA, "utf8_database.csv")
-    with io.open(database_path, "r") as database_file:
+    with (utils.TESTDATA/"utf8_database.csv").open("r") as database_file:
         reader = csv.DictReader(database_file)
         context = next(reader)
     sender, recipients, message = template_message.render(context)
diff --git a/tests/testdata/attachment_template.txt b/tests/testdata/attachment_template.txt
index 0d7a9aa..721d85b 100644
--- a/tests/testdata/attachment_template.txt
+++ b/tests/testdata/attachment_template.txt
@@ -4,7 +4,6 @@ FROM: My Self <myself@mydomain.com>
 ATTACHMENT: attachment_1.txt
 ATTACHMENT: attachment_2.pdf
 ATTACHMENT: attachment_{{number}}.txt
-ATTACHMENT: 
 
 Hi, {{name}},
 
diff --git a/tests/testdata/attachment_template_empty.txt b/tests/testdata/attachment_template_empty.txt
new file mode 100644
index 0000000..9aa3c4a
--- /dev/null
+++ b/tests/testdata/attachment_template_empty.txt
@@ -0,0 +1,6 @@
+TO: a@test.com
+SUBJECT: Hello world
+FROM: b@test.com
+ATTACHMENT: 
+
+Hello world
diff --git a/tests/utils.py b/tests/utils.py
index 66bc745..390429b 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -3,9 +3,14 @@ Utilies common to multiple tests.
 
 Andrew DeOrio <awdeorio@umich.edu>
 """
-import os
+
+# Python 2 pathlib support requires backport
+try:
+    from pathlib2 import Path
+except ImportError:
+    from pathlib import Path
 
 
 # Directories containing test input files
-TESTDIR = os.path.dirname(__file__)
-TESTDATA = os.path.join(TESTDIR, "testdata")
+TESTDIR = Path(__file__).resolve().parent
+TESTDATA = TESTDIR / "testdata"