Sendgrid + Mako email templates quick code review

Question

I created small module to help me create rich sendgrid html emails using the mako templating engine. Any feedback on the design would be appreciated:

makosg.py

from sendgrid.message import Message
from mako.template import Template
from mako.lookup import TemplateLookup

class MakoMessage(Message):

    def __init__(self, addr_from, subject, vars={}, text="",
                 temp_path=None, temp_filename=None):
        html = self.render_template_output(vars, temp_path, temp_filename)
        subject = self._render(subject, vars)
        text = self._render(text, vars)
        super(MakoMessage, self).__init__(addr_from, subject, text, html)

    def render_template_output(self, vars, temp_path, temp_filename):
        if temp_path and temp_filename:
            lookup = TemplateLookup([temp_path])
            template = lookup.get_template(temp_filename)
            return template.render(**vars)            
        return ""

    def _render(self, temp_data, vars):
        template = Template(temp_data)
        return template.render(**vars)

tests.py

import unittest


class MakoMessageTests(unittest.TestCase):

    def _callUT(self, addr_from, subject, vars={}, text="", temp_path=None,
                temp_filename=None):
        from makosg import MakoMessage
        return MakoMessage(addr_from, subject, vars, text, temp_path,
                           temp_filename)

    def test_should_render_basic_txt_template(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'bob'},
                               text="How's it going ${name}?",
                               )
        self.assertTrue('bob' in message.subject, message.subject)
        self.assertTrue('bob' in message.text, message.text)
        self.assertTrue(not message.html, message.html)

    def test_should_render_html_file(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'jim'},
                               text="",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example.mako')
        self.assertTrue('<p>jim</p>' in message.html, message.html)
        self.assertTrue(not message.text, message.text)

    def test_should_render_rich_html_message(self):
        message = self._callUT('test@example.com', 'Hello ${name}',
                               vars={'name': 'Brian', 'fruits':
                                     ['apple', 'pear', 'berry']},
                               text="My favorite fruit is an ${fruits[0]}",
                               temp_path='./makosg/test_templates/',
                               temp_filename='example2.mako')
        self.assertTrue('<li>apple</li>' in message.html and \
                        '<li>pear</li>' in message.html and \
                        '<li>berry</li>' in message.html and \
                        '<p>Hello Brian</p>' in message.html, message.html)
        self.assertTrue('apple' in message.text, message.text)
        self.assertTrue('Brian' in message.subject, message.subject)

https://github.com/bcajes/makosg

Answer 1

Here a few things I spotted (not necessarily design related):

• vars is a builtin, and should be avoided as a variable name. Many template systems call this the context (what you are calling vars), so that's probably a better name.

• Defaulting function parameters to mutable objects (as you do in the line below) is dangerous (can produce unexpected results, if you're not familiar with how python handles function-param defaults). See Why are default arguments evaluated at definition time in Python? For more information.

  def __init__(self, addr_from, subject,context={},text="",

  Instead, you should do something like this:

  def __init__(self, addr_from, subject, context=None, text="",
               temp_path=None, temp_filename=None):
      context = context or {}
  

• I'm not sure the if temp_path ... / return "" is the best design. It took me a bit longer than it should to figure out why you were doing that. One alternative would be to move the conditional into the __init__ function, so it doesn't look like an error is being handled with return "".