Extension docs server: APIDataSource

chrome/common/extensions/docs/server2/api_data_source.py

Index: chrome/common/extensions/docs/server2/api_data_source.py
diff --git a/chrome/common/extensions/docs/server2/api_data_source.py b/chrome/common/extensions/docs/server2/api_data_source.py
new file mode 100644
index 0000000000000000000000000000000000000000..8b857f2873aee0339892451451e98b8cc76cf09d
--- /dev/null
+++ b/chrome/common/extensions/docs/server2/api_data_source.py
@@ -0,0 +1,55 @@
+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import logging
+import os
+import time
+
+import third_party.json_schema_compiler.json_comment_eater as json_comment_eater
+import third_party.json_schema_compiler.model as model
+
+class APIDataSource(object):

[not at google - send to devlin, 2012/06/10 06:58:22]

document

[cduvall, 2012/06/18 19:13:46]

Done.

+  def __init__(self, fetcher, base_paths, cache_timeout_seconds):
+    logging.info('API data source created: %s %d' %
+        (' '.join(base_paths), cache_timeout_seconds))
+    self._fetcher = fetcher
+    self._api_cache = {}
+    self._base_paths = base_paths
+    self._cache_timeout_seconds = cache_timeout_seconds
+
+  class _CachedAPI(object):

[not at google - send to devlin, 2012/06/10 06:58:22]

This caching logic is basically the same as TemplateDataSource, and presumably
you'll need to replicate it at lease once more.  Could you pull this caching
class elsewhere?  One possibility would be something like (taking inspiration
from the guava libraries):

class FetcherCache(object):
  """Banana.
  """

  class Builder(object):
    """Bananarama.
    """
    def __init__(self, fetcher, timeout_seconds):
      self._fetcher = fetcher
      self._timeout_seconds = timeout_seconds

    def build(self, populate_function):
      return FetcherCache(fetcher, timeout_seconds, populate_function)

  # Same as _CachedAPI/_CachedTemplate.
  class _CacheEntry:
    ...

  def __init__(self, fetcher, timeout_seconds, populate_function):
    self._fetcher = fetcher
    self._timeout_seconds = timeout_seconds
    self._populate_function = populate_function

  def get(self, key):
   # Same expiry logic, except rather than creating loaded_api /
   # compiled_template here, create it with populate_function.

APIDataSource and TemplateDataSource would then be given a FetcherCache.Builder
in the constructor, no need to inject the fetcher nor cache_timeout_seconds. 
Something like

--- echo_handler.py _GetInstanceForBranch
...
if branch == 'local':
  fetcher = LocalFetcher(...)
  timeout_seconds = 0
else:
  fetcher = SvnFetcher(...)
  timeout_seconds = 300
cache_builder = FetcherCache.Builder(fetcher, timeout_seconds)
template_data_source = TemplateDataSource(
    cache_builder,
    [..., ...])
api_data_source = APIDataSource(
    cache_builder,
    [...])

--- template_data_source.py

def CompileTemplate(template):
  ...

class TempateDataSource:
  def __init__(self, cache_builder base_paths):
    self._cache = cache_builder.build(CompileTemplate)
    self._base_paths = base_paths

  def get(self, key):
    path = ...
    for base_path in _base_paths:
      try:
        return self._cache.get(path)
      except:
        pass
    return None

[cduvall, 2012/06/18 19:13:46]

Done.

+    def __init__(self, api, expiry):
+      self.api = api

[not at google - send to devlin, 2012/06/10 06:58:22]

private members should be _api

[cduvall, 2012/06/18 19:13:46]

Done.

+      self._expiry = expiry
+
+    def HasExpired(self):
+      return time.time() > self._expiry
+
+  def __getitem__(self, key):
+    return self.get(key)
+
+  def get(self, key):
+    path, ext = os.path.splitext(key)
+    key = model._UnixName(path)

[not at google - send to devlin, 2012/06/10 06:58:22]

overwriting the values of parameters confuses me, assign to different variable? 
Something more descriptive than "key" perhaps...

Also if you want to call _UnixName you might want to make it public, so, remove
the leading _.

[cduvall, 2012/06/18 19:13:46]

Done.

+    path = key + '.json'
+    if key in self._api_cache:
+      if self._api_cache[key].HasExpired():
+        self._api_cache.pop(key)
+    if key not in self._api_cache:
+      logging.info('API cache miss for: ' + path)
+      loaded_api = None
+      for base_path in self._base_paths:
+        try:
+          api = self._fetcher.FetchResource(base_path + path).content

[not at google - send to devlin, 2012/06/10 06:58:22]

No path joining or something?

Oh I see, the paths are set up in echo_handler to have trailing slashes.  I
think that those paths should be directories, so no trailing slash, and do path
joining here.

[cduvall, 2012/06/18 19:13:46]

Done.

+          # The APIs are stored in a list, so get the api out of the list.
+          loaded_api = json.loads(json_comment_eater.Nom(api))[0]
+          self._api_cache[key] = self._CachedAPI(
+              loaded_api,

[not at google - send to devlin, 2012/06/10 06:58:22]

So you're only caching the JSON?  Are you planning to cache the
json-schema-compiler model later?

[cduvall, 2012/06/18 19:13:46]

Yeah for now I'm just caching the JSON, but once the json schema compiler stuff
is set up, it won't be hard to switch to caching that instead.

+              time.time() + self._cache_timeout_seconds)
+          break
+        except:
+          pass
+
+    return loaded_api

[not at google - send to devlin, 2012/06/10 06:58:22]

Isn't this only getting set on a cache miss?

Oh, and same in template_data_source?

[cduvall, 2012/06/18 19:13:46]

Done.