Extensions docs server: Design changes, partial template support.

Die build.py, Die: Part 3 (Design changes, partial template support)

The TemplateFetcher class is now called TemplateDataSource. TemplateDataSource
works with Handlebar.

The Server now keeps a cache of Instances used to render pages (as suggested in
the previous CL).

Part 2: http://codereview.chromium.org/10500004/

BUG=131095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141157

[cduvall, 2012-06-06 22:42:39]

This patch implements the design changes discussed in the last CL. I think I
covered all the suggestions, but if not just let me know. Thanks!

[Aaron Boodman, 2012-06-06 23:47:22]

I think it's time to start giving these CLs proper descriptions, rather than
'die build.py, die: part x'

[Aaron Boodman, 2012-06-06 23:49:58]

exciting!

https://chromiumcodereview.appspot.com/10545043/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10545043/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/echo_handler.py:1: #!/usr/bin/env python
Is the name 'echo_handler.py' still correct for this file?

https://chromiumcodereview.appspot.com/10545043/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/echo_handler.py:36: class
Instance(object):
Consider moving this into its own class. Easier to test.

[clintstaley_gmail.com, 2012-06-06 23:56:41]

It was always meant as a temporary whimsical description. :)  What more 
serious alternative would you like?

Clint

On 6/6/2012 4:47 PM, aa@chromium.org wrote:
> I think it's time to start giving these CLs proper descriptions, 
> rather than
> 'die build.py, die: part x'
>
> https://chromiumcodereview.appspot.com/10545043/
> .
>

[not at google - send to devlin, 2012-06-07 04:43:57]

A lot there!  I'm still getting a grips on the server, sorry :)

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:36: class
Instance(object):
On 2012/06/06 23:49:59, Aaron Boodman wrote:
> Consider moving this into its own class. Easier to test.

Yes please. Might make sense to rename ServerInstance or something when you do.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:40: self._template_util =
TemplateUtility()
Usually it doesn't make sense to instantiate util classes, looking at
TemplateUtility, RenderTemplate (and any future util methods) should just be
def'd at the top level then imported without the "from".

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:75: cache_timeout = 300
nit: cache_timeout_seconds

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:77: [PUBLIC_TEMPLATE_PATH,
PRIVATE_TEMPLATE_PATH], cache_timeout)
nit: parameters all on same line, or all on vertically aligned separate lines.
So, here... vertical obviously :)

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:82: channel_name =
self._branch_util.GetChannelNameFromPath(path)
ditto comment about instantiating util classes.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:86: instance.Run(path,
self)
nit: can inline instance

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:100:
run_wsgi_app(application)
wondering if this a python or appengine idiom or something, but why create the
application outside main? could this be just

def main():
  handlers = [
    ('/*', Server)
  ]
  run_wsgi_app(webapp.WSGIApplication(handlers), debug=False)

btw why not debug=True, just curious?

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:12: self._base_path =
os.path.join(*base_path.split('/'))
Could you combine this and the stuff on line 24 into a separate function?

Also, I think that path.replace('/', os.sep) might be simpler.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:19: def _ReadFile(self,
filename):
Perhaps make this prepend base_path itself, rather than threading through an
instance variable into an instance method (which is redundant). You could also
do the logging here and save needing to os.path.join twice.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:27: result.content =
self._ReadFile(os.path.join(self._base_path, real_path))
Just pass content through to Resource in __init__ ?

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/subversion_fetcher.py:14: self._urlfetch =
urlfetch
url_fetcher ?

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/subversion_fetcher.py:24: return result
nit: inline url and result

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:11: class
TemplateDataSource(object):
I was thinking, should be build all the template rendering into here? All we do
when we render something is
1. grab a template from here
2. render it using TemplateUtility

does it make sense to expose a method like Render() on this which sets up that
"templates" path correctly?  Namely... pointing at itself?  Like

class TemplateDataSource(object):
  ...

  def Render(self, template_name, context):
    template = get(template_name)
    if not template:
      return ''
    # TODO: error handling.
    return template.render(context, { "templates": self }).text

Then we can delete TemplateUtility, which is good, cos I hate util classes.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:12: def
__init__(self, fetcher, base_paths, cache_timeout):
cache_timeout_seconds

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:13:
logging.info('Template data source created.')
It's ok if you want to leave logging in for now (though when the server hits
production we'll need to sanitise it a bit), but you might want to log
base_paths and cache_timeout too.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:38:
self._template_cache[key] = (compiled_template, time.time())
Consider making the cache entries actual objects that know when they timeout,
and store those in _template_cache instead.  Who knows, you might want to put
more metadata in there; it's also a bit easier to read then using tuples IMO.

class _CachedTemplate(object):
  def __init__(self, template, expiry):
    self.template = template
    self._expiry = expiry

  def HasExpired(self):
    return time.time() > self._expiry

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source_test.py:19: class
TestFetcher(LocalFetcher):
why do you need a TestFetcher, can you just use a LocalFetcher directly?

(you should be avoiding inheritance if possible, and certainly not reaching into
subclasses private methods/variables)

[not at google - send to devlin, 2012-06-07 04:46:27]

Btw could you explain the changes you made to Handlebar, then I'll submit that
into the github repo and port across into third_party rather than needing to
change it in this patch.

[cduvall, 2012-06-08 00:39:02]

On 2012/06/07 04:46:27, kalman wrote:
> Btw could you explain the changes you made to Handlebar, then I'll submit that
> into the github repo and port across into third_party rather than needing to
> change it in this patch.

I just made Handlebar check if the context has a get method instead of checking
if it is a dictionary. It looks like 'get' is the only method used from context,
so this shouldn't break anything else. This was needed because the
TemplateDataSource class doesn't extend dict, it just provides a get method.

[cduvall, 2012-06-08 00:39:23]

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:36: class
Instance(object):
On 2012/06/07 04:43:57, kalman wrote:
> On 2012/06/06 23:49:59, Aaron Boodman wrote:
> > Consider moving this into its own class. Easier to test.
> 
> Yes please. Might make sense to rename ServerInstance or something when you
do.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:40: self._template_util =
TemplateUtility()
On 2012/06/07 04:43:57, kalman wrote:
> Usually it doesn't make sense to instantiate util classes, looking at
> TemplateUtility, RenderTemplate (and any future util methods) should just be
> def'd at the top level then imported without the "from".

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:75: cache_timeout = 300
On 2012/06/07 04:43:57, kalman wrote:
> nit: cache_timeout_seconds

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:77: [PUBLIC_TEMPLATE_PATH,
PRIVATE_TEMPLATE_PATH], cache_timeout)
On 2012/06/07 04:43:57, kalman wrote:
> nit: parameters all on same line, or all on vertically aligned separate lines.
> So, here... vertical obviously :)

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:82: channel_name =
self._branch_util.GetChannelNameFromPath(path)
On 2012/06/07 04:43:57, kalman wrote:
> ditto comment about instantiating util classes.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:86: instance.Run(path,
self)
On 2012/06/07 04:43:57, kalman wrote:
> nit: can inline instance

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:100:
run_wsgi_app(application)
On 2012/06/07 04:43:57, kalman wrote:
> wondering if this a python or appengine idiom or something, but why create the
> application outside main? could this be just
> 
> def main():
>   handlers = [
>     ('/*', Server)
>   ]
>   run_wsgi_app(webapp.WSGIApplication(handlers), debug=False)
> 
> btw why not debug=True, just curious?

This is just how the old server was set up. I like your way better I'll change
it to that.

And I'm not sure what the debug setting will change, I just left it on
debug=False to limit unnecessary output.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/local_fetcher.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:12: self._base_path =
os.path.join(*base_path.split('/'))
On 2012/06/07 04:43:57, kalman wrote:
> Could you combine this and the stuff on line 24 into a separate function?
> 
> Also, I think that path.replace('/', os.sep) might be simpler.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:19: def _ReadFile(self,
filename):
On 2012/06/07 04:43:57, kalman wrote:
> Perhaps make this prepend base_path itself, rather than threading through an
> instance variable into an instance method (which is redundant). You could also
> do the logging here and save needing to os.path.join twice.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_fetcher.py:27: result.content =
self._ReadFile(os.path.join(self._base_path, real_path))
On 2012/06/07 04:43:57, kalman wrote:
> Just pass content through to Resource in __init__ ?

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/subversion_fetcher.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/subversion_fetcher.py:14: self._urlfetch =
urlfetch
On 2012/06/07 04:43:57, kalman wrote:
> url_fetcher ?

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/subversion_fetcher.py:24: return result
On 2012/06/07 04:43:57, kalman wrote:
> nit: inline url and result

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:11: class
TemplateDataSource(object):
On 2012/06/07 04:43:57, kalman wrote:
> I was thinking, should be build all the template rendering into here? All we
do
> when we render something is
> 1. grab a template from here
> 2. render it using TemplateUtility
> 
> does it make sense to expose a method like Render() on this which sets up that
> "templates" path correctly?  Namely... pointing at itself?  Like
> 
> class TemplateDataSource(object):
>   ...
> 
>   def Render(self, template_name, context):
>     template = get(template_name)
>     if not template:
>       return ''
>     # TODO: error handling.
>     return template.render(context, { "templates": self }).text
> 
> Then we can delete TemplateUtility, which is good, cos I hate util classes.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:12: def
__init__(self, fetcher, base_paths, cache_timeout):
On 2012/06/07 04:43:57, kalman wrote:
> cache_timeout_seconds

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:13:
logging.info('Template data source created.')
On 2012/06/07 04:43:57, kalman wrote:
> It's ok if you want to leave logging in for now (though when the server hits
> production we'll need to sanitise it a bit), but you might want to log
> base_paths and cache_timeout too.

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source.py:38:
self._template_cache[key] = (compiled_template, time.time())
On 2012/06/07 04:43:57, kalman wrote:
> Consider making the cache entries actual objects that know when they timeout,
> and store those in _template_cache instead.  Who knows, you might want to put
> more metadata in there; it's also a bit easier to read then using tuples IMO.
> 
> class _CachedTemplate(object):
>   def __init__(self, template, expiry):
>     self.template = template
>     self._expiry = expiry
> 
>   def HasExpired(self):
>     return time.time() > self._expiry

Done.

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10545043/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_data_source_test.py:19: class
TestFetcher(LocalFetcher):
On 2012/06/07 04:43:57, kalman wrote:
> why do you need a TestFetcher, can you just use a LocalFetcher directly?
> 
> (you should be avoiding inheritance if possible, and certainly not reaching
into
> subclasses private methods/variables)

Done.

[not at google - send to devlin, 2012-06-08 01:42:53]

Just fairly superficial comments left. LGTM after those.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:10: class
ServerInstance(object):
Comment?

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:27: # TODO should be an
actual not found page.
Should make the response code 404, too.

I presume that without setting the response it defaults to 200, so setting it on
success isn't necessary?  Something to check.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:26: return
template.render(json.loads(context), {'templates': self}).text
nice!

You should add a quick comment around here somewhere explaining this behaviour.
I think just on Render will be fine.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:31: self._expiry =
time.time() + cache_timeout_seconds
I think that we should inject the expiry directly (i.e. pass through the expiry
time, not the cache timeout).  Because the cache timeout is a property of the
TemplateDataSource, and the expiry is a property of CachedTemplate.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:22:
self.assertEquals(_ReadFile(os.path.join(base_path, name + '.html')),
I think that these reference files should be called "_expected.html".

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:24:
os.path.join(base_path, name + '.json'))))
nit: vertical align (I think this will become easier when you have
_ReadLocalFile).

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:28: fetcher =
LocalFetcher(path)
Do python unit tests have a SetUp-type thing?  I think that path (as _base_path)
and fetcher could go in there, they're constants across all tests.

Would also mean that you wouldn't need to thread "path" through everywhere.

Also, _ReadFile could go on TemplateDataSourceTest and know about the base path
directly (no os.path.join noise either).  Maybe call _ReadLocalFile or
something.

[not at google - send to devlin, 2012-06-08 01:43:49]

(btw: this is really cool)

[cduvall, 2012-06-08 02:18:05]

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/server_instance.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:10: class
ServerInstance(object):
On 2012/06/08 01:42:53, kalman wrote:
> Comment?

Done.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/server_instance.py:27: # TODO should be an
actual not found page.
On 2012/06/08 01:42:53, kalman wrote:
> Should make the response code 404, too.
> 
> I presume that without setting the response it defaults to 200, so setting it
on
> success isn't necessary?  Something to check.

Done. Just checked and default is 200.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:26: return
template.render(json.loads(context), {'templates': self}).text
On 2012/06/08 01:42:53, kalman wrote:
> nice!
> 
> You should add a quick comment around here somewhere explaining this
behaviour.
> I think just on Render will be fine.

Done.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source.py:31: self._expiry =
time.time() + cache_timeout_seconds
On 2012/06/08 01:42:53, kalman wrote:
> I think that we should inject the expiry directly (i.e. pass through the
expiry
> time, not the cache timeout).  Because the cache timeout is a property of the
> TemplateDataSource, and the expiry is a property of CachedTemplate.

Done.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:22:
self.assertEquals(_ReadFile(os.path.join(base_path, name + '.html')),
On 2012/06/08 01:42:53, kalman wrote:
> I think that these reference files should be called "_expected.html".

Done.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:24:
os.path.join(base_path, name + '.json'))))
On 2012/06/08 01:42:53, kalman wrote:
> nit: vertical align (I think this will become easier when you have
> _ReadLocalFile).

Done.

http://codereview.chromium.org/10545043/diff/9002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:28: fetcher =
LocalFetcher(path)
On 2012/06/08 01:42:53, kalman wrote:
> Do python unit tests have a SetUp-type thing?  I think that path (as
_base_path)
> and fetcher could go in there, they're constants across all tests.
> 
> Would also mean that you wouldn't need to thread "path" through everywhere.
> 
> Also, _ReadFile could go on TemplateDataSourceTest and know about the base
path
> directly (no os.path.join noise either).  Maybe call _ReadLocalFile or
> something.

Good idea. It helped with base_path, but LocalFetcher had to be called later
because it needs the full base_path to find the files.

[not at google - send to devlin, 2012-06-08 02:23:44]

(just an FYI, I know you've already submitted it, I just looked again out of
curiosity)

http://codereview.chromium.org/10545043/diff/3004/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

http://codereview.chromium.org/10545043/diff/3004/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_data_source_test.py:27:
self._ReadLocalFile(name + '.json')))
Btw, in general, if you think it's more readable to have something like

    self.assertEquals(
        self._ReadLocalFile(name + '_expected.html'),
        data_source.Render(template_name, self._ReadLocalFile(name + '.json')))

then that's fine too. All nits, so much of a muchness.

[cduvall, 2012-06-08 02:24:49]

On 2012/06/08 02:23:44, kalman wrote:
> (just an FYI, I know you've already submitted it, I just looked again out of
> curiosity)
> 
>
http://codereview.chromium.org/10545043/diff/3004/chrome/common/extensions/do...
> File chrome/common/extensions/docs/server2/template_data_source_test.py
(right):
> 
>
http://codereview.chromium.org/10545043/diff/3004/chrome/common/extensions/do...
> chrome/common/extensions/docs/server2/template_data_source_test.py:27:
> self._ReadLocalFile(name + '.json')))
> Btw, in general, if you think it's more readable to have something like
> 
>     self.assertEquals(
>         self._ReadLocalFile(name + '_expected.html'),
>         data_source.Render(template_name, self._ReadLocalFile(name +
'.json')))
> 
> then that's fine too. All nits, so much of a muchness.

Ahh ok. I just didn't really know what to do with the indenting on that one.