Die build.py, Die: Part 2 (LocalFetcher, Handlebar support, build script)

Die build.py, Die: Part 2 (LocalFetcher, Handlebar support, build script)

I implemented/tested the local fetcher and minimal template support.

I also wrote a script that will run the server in the local file system, and as
a bonus, clean itself up on ^C.

Part 1: http://codereview.chromium.org/10387225/

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

[cduvall, 2012-06-01 22:57:19]

Here's my initial upload of my most recent work.

[Aaron Boodman, 2012-06-02 07:50:22]

Sweet.

+kalman, fyi

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:9: THIRD_PARTY_DIR =
sys.path[0] + '/../../../../../third_party/'
Not sure if this will work on Windows. Use os.join() to get the right separators
automatically.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:10: LOCAL_THIRD_PARTY_DIR
= sys.path[0] + '/third_party/'
For added safety, do a sanity check that third_party is there. If not, warn that
probably not running from correct directory.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:17: f.write('\n')
You can just do os.utime(path).

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:21: f.write('\nfrom
handlebar import Handlebar\n')
I think it would be less surprising to just import this in the files it is used.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:24: DOCS_PATH =
'chrome/common/extensions/docs/'
os.path.join() ... all throughout this code.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:40: # argument to toggle
this.
They don't show up in sys.argv?

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:13: """Class to fetch
docs from src.chromium.org.
It will fetch lots of things. How about s/docs/resources/

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:28: class
LocalFetcher(object):
Each class should typically be in a separate file with the same name, even if
they are small. Sorry I didn't notice this last time.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:32: def
__init__(self):
It seems like it would be more robust to pass in a base path to fetch from.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher_test.py:9: from
resource_fetcher import SubversionFetcher, LocalFetcher
Imports should be on separate lines:

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_format...

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/start_dev_server.py:25:
signal.signal(signal.SIGINT, CleanUp)
:)

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/start_dev_server.py:28:
os.chdir(sys.path[0] + '/../../../../../')
chdir is kinda like a global variable. Is it possible to just reference things
using their full paths, rather than assuming the current dir?

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_utility.py:10: def
RenderTemplate(self, json_str, content):
Nit: It would be more intuitive to me if the template string came first. Also,
name that param 'template_source'? Or just 'source'?

[cduvall, 2012-06-02 19:12:04]

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:9: THIRD_PARTY_DIR =
sys.path[0] + '/../../../../../third_party/'
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> Not sure if this will work on Windows. Use os.join() to get the right
separators
> automatically.

Done.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:10: LOCAL_THIRD_PARTY_DIR
= sys.path[0] + '/third_party/'
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> For added safety, do a sanity check that third_party is there. If not, warn
that
> probably not running from correct directory.

sys.path[0] allows this to work when run from any directory, but I'll add the
check anyway for safety. I'm just checking the third_party dir under src/
because the local one is created every run.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:17: f.write('\n')
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> You can just do os.utime(path).

Done.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:21: f.write('\nfrom
handlebar import Handlebar\n')
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> I think it would be less surprising to just import this in the files it is
used.

This import is needed to access the class (I think because Handlebar is not in
the same directory as the rest of the files). In the other files, there is still
a 'from third_party.handlebar import Handlebar', so it shouldn't be too
confusing. It can't find the name Handlebar without this import in __init__.py.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:24: DOCS_PATH =
'chrome/common/extensions/docs/'
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> os.path.join() ... all throughout this code.

So these paths can be used as URLs or as file paths depending on what kind of
resource fetcher is used. That means that if your running this on windows it
will put '\' in all your URLs. I believe this will make it an illegal URL
(although most browsers will probably handle it). I'm 99% sure that windows can
handle '/', and since we're not using any absolute paths here, it shouldn't be a
problem on windows.

But I'm fine with os.path.join, or what I have here. Which do you think is
better?

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:40: # argument to toggle
this.
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> They don't show up in sys.argv?

Nope, since dev_appserver.py calls this script separately (I'm assuming with
something like subprocess.call), is doesn't inherit sys.argv. This was a common
workaround I found online.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:13: """Class to fetch
docs from src.chromium.org.
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> It will fetch lots of things. How about s/docs/resources/

Done.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:28: class
LocalFetcher(object):
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> Each class should typically be in a separate file with the same name, even if
> they are small. Sorry I didn't notice this last time.

Done.

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher.py:32: def
__init__(self):
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> It seems like it would be more robust to pass in a base path to fetch from.

Done.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/resource_fetcher_test.py:9: from
resource_fetcher import SubversionFetcher, LocalFetcher
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> Imports should be on separate lines:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_format...

Done.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/start_dev_server.py:28:
os.chdir(sys.path[0] + '/../../../../../')
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> chdir is kinda like a global variable. Is it possible to just reference things
> using their full paths, rather than assuming the current dir?

Done.

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/template_utility.py:10: def
RenderTemplate(self, json_str, content):
On 2012/06/02 07:50:22, Aaron Boodman wrote:
> Nit: It would be more intuitive to me if the template string came first. Also,
> name that param 'template_source'? Or just 'source'?

Done.

[Aaron Boodman, 2012-06-03 05:43:14]

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/build_server.py:10: LOCAL_THIRD_PARTY_DIR
= sys.path[0] + '/third_party/'
On 2012/06/02 19:12:05, cduvall wrote:
> On 2012/06/02 07:50:22, Aaron Boodman wrote:
> > For added safety, do a sanity check that third_party is there. If not, warn
> that
> > probably not running from correct directory.
> 
> sys.path[0] allows this to work when run from any directory, but I'll add the
> check anyway for safety. I'm just checking the third_party dir under src/
> because the local one is created every run.

Ah, my mistake. I guess the check is not necessary then. My only goal is that it
either works from any directory, or tells you when run from wrong directory (in
which case it should tell you which directory it would like to be run from).

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:24: DOCS_PATH =
'chrome/common/extensions/docs/'
On 2012/06/02 19:12:05, cduvall wrote:
> On 2012/06/02 07:50:22, Aaron Boodman wrote:
> > os.path.join() ... all throughout this code.
> 
> So these paths can be used as URLs or as file paths depending on what kind of
> resource fetcher is used. That means that if your running this on windows it
> will put '\' in all your URLs. I believe this will make it an illegal URL
> (although most browsers will probably handle it). I'm 99% sure that windows
can
> handle '/', and since we're not using any absolute paths here, it shouldn't be
a
> problem on windows.
> 
> But I'm fine with os.path.join, or what I have here. Which do you think is
> better?

I see. When you use it as a path, can you split by '/' and then join by os.path
so that we know it's definitely correct?

[not at google - send to devlin, 2012-06-04 04:37:08]

Thanks.  Browsed over everything, just have a couple of drive-by comments.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/build_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/build_server.py:5: 
I was totally confused here.  Please add a comment explaining what this file is
for.

Speaking of which, presumably we'll have some kind of deploy script somewhere. 
Could this stuff be added to that?

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/start_dev_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/start_dev_server.py:15: SRC_PATH =
os.path.join(SERVER_PATH, '..', '..', '..', '..', '..')
pardir?

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/start_dev_server.py:28:
build_server.main()
This means that starting the dev server involves copying some stuff into third
party etc.  Is there a way to avoid that and just set up the python path?

i.e. avoid running build_server.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_utility.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_utility.py:14: return
rendered_template
nit: we'll probably want to do something with the errors at some point (e.g. log
them) so assign the result of .render to something (e.g. "result") then return
result.text.

and a TODO for dealing with the errors.

actually: I think this file is a bit dangerous, since compiling templates is
expensive and this is encouraging laziness.  We should be caching the compiled
templates.  So instead, could we make a wrapper around resource fetching like
HandelbarFetcher / TemplateFetcher/ whatever which fetches paths then caches the
*compiled* result?

I guess there will be issues with knowing when to refresh the cache, hopefully,
ones that are surmountable.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/test_data/template_utility/test2.html
(right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/test_data/template_utility/test2.html:1:
This 2nd test is an incredible, amazing, awesome, test.
sgtm!

[Aaron Boodman, 2012-06-04 06:29:09]

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/build_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/build_server.py:5: 
On 2012/06/04 04:37:08, kalman wrote:
> I was totally confused here.  Please add a comment explaining what this file
is
> for.
> 
> Speaking of which, presumably we'll have some kind of deploy script somewhere.

> Could this stuff be added to that?

The two scripts do different things:

- build_server.py assembles the package of files that can be uploaded to app
engine.
- start_dev_server.py starts the server for local development.

We could put them in one file, but I don't see why that is desirable.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/start_dev_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/start_dev_server.py:28:
build_server.main()
On 2012/06/04 04:37:08, kalman wrote:
> This means that starting the dev server involves copying some stuff into third
> party etc.  Is there a way to avoid that and just set up the python path?
> 
> i.e. avoid running build_server.

There are going to be non-Python dependencies, like third-party JavaScript
files.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_utility.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_utility.py:14: return
rendered_template
On 2012/06/04 04:37:08, kalman wrote:
> nit: we'll probably want to do something with the errors at some point (e.g.
log
> them) so assign the result of .render to something (e.g. "result") then return
> result.text.
> 
> and a TODO for dealing with the errors.
> 
> actually: I think this file is a bit dangerous, since compiling templates is
> expensive and this is encouraging laziness.  We should be caching the compiled
> templates.  So instead, could we make a wrapper around resource fetching like
> HandelbarFetcher / TemplateFetcher/ whatever which fetches paths then caches
the
> *compiled* result?
> 
> I guess there will be issues with knowing when to refresh the cache,
hopefully,
> ones that are surmountable.

This makes sense because we're going to end up with some sub-templates that are
reused over and over.

Note that memcached can only cache serializable objects, so I don't think that's
going to work here. The templates can only be cached within a process.

[not at google - send to devlin, 2012-06-04 06:36:29]

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/template_utility.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/template_utility.py:14: return
rendered_template
On 2012/06/04 06:29:09, Aaron Boodman wrote:
> On 2012/06/04 04:37:08, kalman wrote:
> > nit: we'll probably want to do something with the errors at some point (e.g.
> log
> > them) so assign the result of .render to something (e.g. "result") then
return
> > result.text.
> > 
> > and a TODO for dealing with the errors.
> > 
> > actually: I think this file is a bit dangerous, since compiling templates is
> > expensive and this is encouraging laziness.  We should be caching the
compiled
> > templates.  So instead, could we make a wrapper around resource fetching
like
> > HandelbarFetcher / TemplateFetcher/ whatever which fetches paths then caches
> the
> > *compiled* result?
> > 
> > I guess there will be issues with knowing when to refresh the cache,
> hopefully,
> > ones that are surmountable.
> 
> This makes sense because we're going to end up with some sub-templates that
are
> reused over and over.
> 
> Note that memcached can only cache serializable objects, so I don't think
that's
> going to work here. The templates can only be cached within a process.

Within each process is certainly a lot better than nothing.

Note that, for future record, in the subtemplate case we'll need to expose a
dictionary interface to the TemplateFetcher class, because is what the templates
will be expecting to see.  Something like

--- bar.html ---
Hello from bar!

--- foo.html ---
This is going to include some other template: {{+partials.bar}}

--- server_code.py ---
template = template_fetcher['foo'];  # whatever the method is called
result = template.render({
  templates: template_fetcher
});
print(result.text)

> ./server_code.py
This is going to include some other template: Hello from bar!

[not at google - send to devlin, 2012-06-04 06:38:54]

Oops slightly screwed up that code example.  Try again:

--- bar.html ---
Hello from bar!

--- foo.html ---
This is going to include some other template: {{+template_fetcher.bar}}

--- server_code.py ---
template = template_fetcher['foo'];  # whatever the method is called
result = template.render({
  templates: template_fetcher
});
print(result.text)

[not at google - send to devlin, 2012-06-04 06:40:19]

Screwed it up again!  Ok this time will be right.


--- bar.html ---
Hello from bar!

--- foo.html ---
This is going to include some other template: {{+template_fetcher.bar}}

--- server_code.py ---
template = template_fetcher['foo'];  # whatever the method is called
result = template.render({
  template_fetcher: template_fetcher
});
print(result.text)

> ./server_code.py
This is going to include some other template: Hello from bar!

[cduvall, 2012-06-04 21:53:25]

I added a TemplateFetcher class to fetch compiled templates and cache them in
the process. This should work well for partial templates too.

How are template files going to know what partial templates they need? Are we
going to have something like an 'include' at the top with the names of all the
necessary partials?

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

http://codereview.chromium.org/10500004/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/echo_handler.py:24: DOCS_PATH =
'chrome/common/extensions/docs/'
On 2012/06/03 05:43:14, Aaron Boodman wrote:
> On 2012/06/02 19:12:05, cduvall wrote:
> > On 2012/06/02 07:50:22, Aaron Boodman wrote:
> > > os.path.join() ... all throughout this code.
> > 
> > So these paths can be used as URLs or as file paths depending on what kind
of
> > resource fetcher is used. That means that if your running this on windows it
> > will put '\' in all your URLs. I believe this will make it an illegal URL
> > (although most browsers will probably handle it). I'm 99% sure that windows
> can
> > handle '/', and since we're not using any absolute paths here, it shouldn't
be
> a
> > problem on windows.
> > 
> > But I'm fine with os.path.join, or what I have here. Which do you think is
> > better?
> 
> I see. When you use it as a path, can you split by '/' and then join by
os.path
> so that we know it's definitely correct?

Done.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/build_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/build_server.py:5: 
On 2012/06/04 06:29:09, Aaron Boodman wrote:
> On 2012/06/04 04:37:08, kalman wrote:
> > I was totally confused here.  Please add a comment explaining what this file
> is
> > for.
> > 
> > Speaking of which, presumably we'll have some kind of deploy script
somewhere.
> 
> > Could this stuff be added to that?
> 
> The two scripts do different things:
> 
> - build_server.py assembles the package of files that can be uploaded to app
> engine.
> - start_dev_server.py starts the server for local development.
> 
> We could put them in one file, but I don't see why that is desirable.

I added a comment for clarity.

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/start_dev_server.py (right):

http://codereview.chromium.org/10500004/diff/2003/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/start_dev_server.py:15: SRC_PATH =
os.path.join(SERVER_PATH, '..', '..', '..', '..', '..')
On 2012/06/04 04:37:08, kalman wrote:
> pardir?

Done.

[cduvall, 2012-06-04 22:25:55]

Local is now a branch like 'dev', 'stable'...

Cleaned up echo_handler.py.

[not at google - send to devlin, 2012-06-04 23:09:51]

To answer your question in the overall comments: yep, it should work well for
partials, that was the hope. I think that if you just plug in one of those
TempateFetchers it should just lazily populate the necessary partials?  I.e. no
includes necessary?  Or am I missing something / misunderstanding?

As a more general comment, I've suggested that we inject the branch into each
TemplateFetcher so that it has a more natural dictionary interface.  Could we do
this for all fetchers?  Rather than passing through the branch, have each
fetcher injected with the branch on construction?  I have a feeling that doing
that will lend itself to cleaner and more modular code in the long run.

I don't think that it's a requirement to landing this particular patch, though.

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_fetcher.py:17: TEMPLATE_CACHE =
{}
See comment below as to what this could be.

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_fetcher.py:21: self.fetcher =
fetcher
should be _fetcher

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_fetcher.py:28: def AsDict(self,
branch):
Yeah, this won't play so well with plugging stuff into templates as a dict.

Rather than this could we have this structured more like...

_FETCHERS = {}

TemplateFetcher(object):
  def __init__(self, fetcher, branch):
    self._fetcher = fetcher
    self._branch = branch

  def __getitem__(self, key):
    return self.FetchTemplate(key)

  def FetchTemplate(self, name):
    path = name + '.html'
    template = self._fetcher.FetchResource(self._branch, path)
    ... etc (what you had)

If necessary have some kind of factory class like

TemplateFetcherFactory(object):
  __init__(self, fetcher):
    self._fetcher = fetcher
    self._cache = {}

  def ForBranch(self, branch):
    if branch in self._cache:
      return self._cache[branch]
    self._cache[branch] = TemplateFetcher(self._fetcher, branch)
    return self._cache[branch]

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_fetcher.py:45: template =
self.fetcher.FetchResource(branch, path).content
Something to think about: this 5 minute thing is a bit sucky if somebody is
iterating very quickly on the templates.

Instead of this, could we store a hash of the template source along with each
template. Then, always fetch the source, compare its hash to what we already
have, and if it's different then recompile the template. I'm not even sure we'd
buy much from caching it, but, yeah.

Come to think of it, we could even optimise this a bit by giving it a 5-second
timeout or something, so that rendering partials doesn't cause a whole bunch of
fetching. But that's something to worry about later if we find things are slow
and this is a major cause of it etc etc.

[not at google - send to devlin, 2012-06-04 23:43:50]

More generally, something like

def Instance(object):
  """Contains the configuration for a single "instance" of the
  server for a given channel. Most of the logic should go in here,
  with the injected dependencies. It's the job of the server
  """
  def __init__(self, fetcher, template_fetcher, logger, ...):
    self._fetcher = fetcher
    self._template_fetcher = template_fetcher
    self._logger = logger

  def Run():
    "All the things.
    "
    ...

def TheServerThing(object):
  def __init__(self):
    self._instances = {}

  def GetInstanceForBranch(self, branch):
    if branch in self._instances:
      return self._instances[branch]
    if branch == 'local':
      # Hack for local stuff.
      fetcher = LocalFetcher()
    else:
      fetcher = Fetcher(branch_utility.blah(branch))
    template_fetcher = TemplateFetcher(fetcher)
    logger = FancyLogger(branch)
    ...
    self_instances[branch] = Instance(fetcher, template_fetcher, logger)
    return self_instances[branch]

  # Oversimplification of whatever the servlet interface is. But yeah.
  def handle(self, path):
    branch = FigureOutBranch(path)
    instance = GetInstanceForBranch(branch)
    instance.Run()

[not at google - send to devlin, 2012-06-04 23:52:26]

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/template_fetcher_test.py (right):

http://codereview.chromium.org/10500004/diff/20001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_fetcher_test.py:31:
t_fetcher.FetchTemplate('b', 'test1.tmpl')
Two little things here: I thought we were doing .html for the templates?

and I'd hoped we could arrange it so that pre-fetching the partials wasn't
necessary, that TemplateFetcher would know how to resolve this.

Btw what do we do when fetching fails for some template (e.g. it doesn't exist)?
 We should probably log and error and return the empty string, so that case
should be tested.

[Aaron Boodman, 2012-06-05 00:31:58]

On 2012/06/04 23:43:50, kalman wrote:
> More generally, something like
> 
> def Instance(object):
>   """Contains the configuration for a single "instance" of the
>   server for a given channel. Most of the logic should go in here,
>   with the injected dependencies. It's the job of the server
>   """
>   def __init__(self, fetcher, template_fetcher, logger, ...):
>     self._fetcher = fetcher
>     self._template_fetcher = template_fetcher
>     self._logger = logger
> 
>   def Run():
>     "All the things.
>     "
>     ...
> 
> def TheServerThing(object):
>   def __init__(self):
>     self._instances = {}
> 
>   def GetInstanceForBranch(self, branch):
>     if branch in self._instances:
>       return self._instances[branch]
>     if branch == 'local':
>       # Hack for local stuff.
>       fetcher = LocalFetcher()
>     else:
>       fetcher = Fetcher(branch_utility.blah(branch))
>     template_fetcher = TemplateFetcher(fetcher)
>     logger = FancyLogger(branch)
>     ...
>     self_instances[branch] = Instance(fetcher, template_fetcher, logger)
>     return self_instances[branch]
> 
>   # Oversimplification of whatever the servlet interface is. But yeah.
>   def handle(self, path):
>     branch = FigureOutBranch(path)
>     instance = GetInstanceForBranch(branch)
>     instance.Run()

I like this... Clark, wdyt?

[cduvall, 2012-06-05 00:38:15]

> I like this... Clark, wdyt?

I like it too. I'll implement this and the TemplateDataSource stuff in the next
CL.

I think this CL should be good to go.

[Aaron Boodman, 2012-06-05 00:50:04]

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/echo_handler.py:61: template =
template_fetcher[PUBLIC_TEMPLATE_PATH + filename]
Like kalman suggested, I think this should return empty string when the path is
not found, so there's no need to try/catch it.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:25: if (self._branch,
key) not in TEMPLATE_CACHE:
Nit: Is there a reason for FetchTemplate() now? It seems like all hte logic
could move here.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
Indexing tuples by number like this always looks ugly to me. Better to get the
values out, like:

template, cache_time = TEMPLATE_CACHE[key]
if (cache_time < ...)

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
Does this work? Seems like you need to subtract from now.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
kalman is right that this is going to be a pain especially when working locally.

I think that in a future CL, you should expose the amount of time to cache as a
property of this class, and set it to zero in the local case.

[not at google - send to devlin, 2012-06-05 00:56:10]

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:25: if (self._branch,
key) not in TEMPLATE_CACHE:
On 2012/06/05 00:50:04, Aaron Boodman wrote:
> Nit: Is there a reason for FetchTemplate() now? It seems like all hte logic
> could move here.

Python style guide reasons?

[cduvall, 2012-06-05 01:07:37]

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:25: if (self._branch,
key) not in TEMPLATE_CACHE:
On 2012/06/05 00:50:04, Aaron Boodman wrote:
> Nit: Is there a reason for FetchTemplate() now? It seems like all hte logic
> could move here.

I moved it for now we can move it back out to FetchTemplate if it gets too big.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
On 2012/06/05 00:50:04, Aaron Boodman wrote:
> Indexing tuples by number like this always looks ugly to me. Better to get the
> values out, like:
> 
> template, cache_time = TEMPLATE_CACHE[key]
> if (cache_time < ...)

Done.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
On 2012/06/05 00:50:04, Aaron Boodman wrote:
> Does this work? Seems like you need to subtract from now.

Done.

https://chromiumcodereview.appspot.com/10500004/diff/19031/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:33: if
TEMPLATE_CACHE[key][1] < CACHE_TIMEOUT:
On 2012/06/05 00:50:04, Aaron Boodman wrote:
> kalman is right that this is going to be a pain especially when working
locally.
> 
> I think that in a future CL, you should expose the amount of time to cache as
a
> property of this class, and set it to zero in the local case.

Agreed, I'll do that in the next CL.

[Aaron Boodman, 2012-06-05 01:20:22]

lgtm

https://chromiumcodereview.appspot.com/10500004/diff/30002/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/30002/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:28: if time.clock() -
compile_time > CACHE_TIMEOUT:
Add parens around the subtraction to make order of operations clear.

[cduvall, 2012-06-05 01:22:03]

https://chromiumcodereview.appspot.com/10500004/diff/30002/chrome/common/exte...
File chrome/common/extensions/docs/server2/template_fetcher.py (right):

https://chromiumcodereview.appspot.com/10500004/diff/30002/chrome/common/exte...
chrome/common/extensions/docs/server2/template_fetcher.py:28: if time.clock() -
compile_time > CACHE_TIMEOUT:
On 2012/06/05 01:20:22, Aaron Boodman wrote:
> Add parens around the subtraction to make order of operations clear.

Done.