Extension docs server: APIDataSource

Extension docs server: APIDataSource

Implemented and tested the APIDataSource class to get info from the JSON APIs.

This will probably use some tools from the JSON schema compiler in the future.

Also made some changes to build_server.py.

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

[cduvall, 2012-06-08 21:05:47]

Thought I would put up a CL for APIDataSource before I started messing with the
JSON schema compiler. Thanks!

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

A fairly major change there... sorry, I got a bit carried away writing the code.

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

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:13: class
APIDataSource(object):
document

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:22: class
_CachedAPI(object):
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

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:24: self.api = api
private members should be _api

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:35: key =
model._UnixName(path)
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 _.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:45: api =
self._fetcher.FetchResource(base_path + path).content
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.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:49: loaded_api,
So you're only caching the JSON?  Are you planning to cache the
json-schema-compiler model later?

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:55: return loaded_api
Isn't this only getting set on a cache miss?

Oh, and same in template_data_source?

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

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/build_server.py:39:
os.path.join(local_schema_compiler_path, filename))
Could you have some kind of CopyThirdParty function or something which makes
this all a bit less hands-on?

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/build_server.py:49: # To be able to use
the Handlebar class we need this import in __init__.py.
Why do you need it in __init__ (I'm curious / lacking in understanding)?

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

On vacation for a week, but now I'm back in action.

Implemented FetcherCache, it looks much better now.

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

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:13: class
APIDataSource(object):
On 2012/06/10 06:58:22, kalman wrote:
> document

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:22: class
_CachedAPI(object):
On 2012/06/10 06:58:22, kalman wrote:
> 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

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:24: self.api = api
On 2012/06/10 06:58:22, kalman wrote:
> private members should be _api

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:35: key =
model._UnixName(path)
On 2012/06/10 06:58:22, kalman wrote:
> 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 _.

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:45: api =
self._fetcher.FetchResource(base_path + path).content
On 2012/06/10 06:58:22, kalman wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:49: loaded_api,
On 2012/06/10 06:58:22, kalman wrote:
> So you're only caching the JSON?  Are you planning to cache the
> json-schema-compiler model later?

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.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:55: return loaded_api
On 2012/06/10 06:58:22, kalman wrote:
> Isn't this only getting set on a cache miss?
> 
> Oh, and same in template_data_source?

Done.

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

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/build_server.py:39:
os.path.join(local_schema_compiler_path, filename))
On 2012/06/10 06:58:22, kalman wrote:
> Could you have some kind of CopyThirdParty function or something which makes
> this all a bit less hands-on?

Done.

https://chromiumcodereview.appspot.com/10546078/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/build_server.py:49: # To be able to use
the Handlebar class we need this import in __init__.py.
On 2012/06/10 06:58:22, kalman wrote:
> Why do you need it in __init__ (I'm curious / lacking in understanding)?

For some reason python won't let you import classes from a different directory
without there being an import in that directory's __init__.py. It gives the
error 'ImportError: cannot import name Handlebar' if this import is not here.

I tried a few different things to get the import to work, but this was the best
solution I found.

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

lgtm with nits

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:16: return
json.loads(json_comment_eater.Nom(api))[0]
nit: either make static / pull outside the class (it doesn't need state so it's
possible to do that), or move below the constructor (constructor is usually the
first thing in a class I believe).

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:19: self._cache =
cache_builder.build(self._LoadAPI)
nice

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/echo_handler.py:33: PRIVATE_TEMPLATE_PATH
= DOCS_PATH + '/template2/private'
these are filesystem paths right? Should be os.path.join then.

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/fetcher_cache.py:9: """A cache for a
fetcher object.
s/fetcher object/fetcher objects/

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/fetcher_cache.py:46: return
self._cache[key]._cache_data
nice

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:12: return
Handlebar(template)
nit: either make this static / pull outside the class, or put it below the
constructor.

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:20: the partial
templates needed with |self._fetcher|. Partials are retrieved
comment needs updating

[cduvall, 2012-06-18 19:48:46]

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/api_data_source.py:16: return
json.loads(json_comment_eater.Nom(api))[0]
On 2012/06/18 19:30:40, kalman wrote:
> nit: either make static / pull outside the class (it doesn't need state so
it's
> possible to do that), or move below the constructor (constructor is usually
the
> first thing in a class I believe).

Done.

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/echo_handler.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/echo_handler.py:33: PRIVATE_TEMPLATE_PATH
= DOCS_PATH + '/template2/private'
On 2012/06/18 19:30:40, kalman wrote:
> these are filesystem paths right? Should be os.path.join then.

These are URLs. When they are used as filesystem paths, they are converted (in
LocalFetcher).

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/fetcher_cache.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/fetcher_cache.py:9: """A cache for a
fetcher object.
On 2012/06/18 19:30:40, kalman wrote:
> s/fetcher object/fetcher objects/

Done.

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
File chrome/common/extensions/docs/server2/template_data_source.py (right):

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:12: return
Handlebar(template)
On 2012/06/18 19:30:40, kalman wrote:
> nit: either make this static / pull outside the class, or put it below the
> constructor.

Done.

https://chromiumcodereview.appspot.com/10546078/diff/8001/chrome/common/exten...
chrome/common/extensions/docs/server2/template_data_source.py:20: the partial
templates needed with |self._fetcher|. Partials are retrieved
On 2012/06/18 19:30:40, kalman wrote:
> comment needs updating

Done.

[commit-bot: I haz the power, 2012-06-18 19:56:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/10546078/12002

[commit-bot: I haz the power, 2012-06-18 19:56:26]

Presubmit check for 10546078-12002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
./template_data_source_test.py failed!
Command ./template_data_source_test.py returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2
Traceback (most recent call last):
  File "./template_data_source_test.py", line 12, in <module>
    from template_data_source import TemplateDataSource
  File
"/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2/template_data_source.py",
line 5, in <module>
    from third_party.handlebar import Handlebar
ImportError: No module named third_party.handlebar


./api_data_source_test.py failed!
Command ./api_data_source_test.py returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2
Traceback (most recent call last):
  File "./api_data_source_test.py", line 12, in <module>
    from api_data_source import APIDataSource
  File
"/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2/api_data_source.py",
line 8, in <module>
    import third_party.json_schema_compiler.json_comment_eater as
json_comment_eater
ImportError: No module named third_party.json_schema_compiler.json_comment_eater


Presubmit checks took 3.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!