Extensions Docs Server: HandlebarDictGenerator

Extensions Docs Server: DictGenerator

Implemented the HandlebarDictGenerator that works with the JSON Schema Compiler to
produce a dictionary that Handlebar can work with.

The HandlebarDictGenerator removes all items marked "nodoc", and only keeps the 
necessary parts of the JSON. $refs can be resolved because, if the
dependencies are in other files, they are formatted as "dependency.Item"
(eg. windows.Window), so the link can be created based on this name.

APIDataSource now uses a DictGenerator instead of the raw JSON APIs.

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

[cduvall, 2012-06-19 18:59:38]

DictGenerator is here. Let me know if this is on the right track.

[not at google - send to devlin, 2012-06-19 20:33:10]

This is the right idea. I didn't look at it too thoroughly since I have some
general comments that might affect things:

1. Checking in actual JSON schema files is an easy way to generate data for
tests, but it's a bit awkward as the JSON schema will diverge and then it's
confusing. Somebody searching for bookmarks.json will find a few different
versions. Which version is the test data meant to affect? Issues like that. I
think there are existing JSON schema tests that do this and it's a shame; there
are also JSON schema tests which write their own, targeted JSON schema stuff and
that works well.

2. As I said, generating a dict from the model is the right idea. However, I
think that when you start actually generating HTML you'll find that some of what
you've put in those dicts isn't needed, and some of it will have to change
dramatically. Have a look at some HTML (e.g.
http://code.google.com/chrome/extensions/beta/storage.html) and imagine what
will need to go in the dict to generate that. Remember that you can't do string
comparisons from the templates, you can only check for existence. I guess what
I'm saying is that by all means submit code which stubs out the mechanism for
generating this stuff, but don't put too much time into actually filling it.
Just make it easier to iterate on it, since this stuff will grow as the docs
necessitate.

3. dict_generator is a bit of a generic name since it's specifically a dict that
is intended to be passed into a template (handlebar specifically) for rendering.
Something like handlebar_dict_generator, handlebar_context_generator,
handlebar_input_generator or something?

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source_test.py:33: expected =
json.loads(self._ReadLocalFile('test_file.json'))[0]
Could you modify "expected" here rather than overriding _LoadAPI for
TestAPIDataSource? It's kind of defeating the purpose of testing APIDataSource
if you're going to override part of its behaviour.

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:30: def
_GenerateType(self, t):
generally I prefer to list functions top-down. So the public methods like
Generate up the top, then _GenerateTypes below that, _GenerateType below that,
etc.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:32: type_dict['name'] =
t.name
"t" -> "type_"

I'd have called "type_dict" just "dict" but no biggie. Also you can initialize
this dict a bit up-front like

type_dict = {
  'name': type_.name,
  'type': type_.type_
}

same for a lot of the things in this file.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:40:
type_dict['item_type'] = self._GenerateType(t.item_type)
It looks like model.py normalises this and always sets properties/functions so
you don't need to check with getattr.

(in fact needing to use getattr everywhere would be a failing of a model type)

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:45: return None
hmmmm I'm not sure whether no-types should be normalised as a empty list or as
None (ditto properties etc). I'd probably say empty-list is probably less to
think about but it's much of a muchness.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:48:
types_list.append(self._GenerateType(types[t]))
try to have names like "type_name" rather than "t".

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:54: if len(c.params) !=
0:
 > 0

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:112: d['properties'] =
self._GenerateProperties(self._namespace.properties)
return {
  'name': self._namespace.name,
  'types': self._GenerateTypes(..)
  ..
}

?

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator_test.py:28: 
extra \n ?

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

I expect to be changing this once I start generating HTML, but I think the
basics are there.

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source_test.py:33: expected =
json.loads(self._ReadLocalFile('test_file.json'))[0]
On 2012/06/19 20:33:10, kalman wrote:
> Could you modify "expected" here rather than overriding _LoadAPI for
> TestAPIDataSource? It's kind of defeating the purpose of testing APIDataSource
> if you're going to override part of its behaviour.

Done.

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:30: def
_GenerateType(self, t):
On 2012/06/19 20:33:10, kalman wrote:
> generally I prefer to list functions top-down. So the public methods like
> Generate up the top, then _GenerateTypes below that, _GenerateType below that,
> etc.

Done.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:32: type_dict['name'] =
t.name
On 2012/06/19 20:33:10, kalman wrote:
> "t" -> "type_"
> 
> I'd have called "type_dict" just "dict" but no biggie. Also you can initialize
> this dict a bit up-front like
> 
> type_dict = {
>   'name': type_.name,
>   'type': type_.type_
> }
> 
> same for a lot of the things in this file.

Done.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:40:
type_dict['item_type'] = self._GenerateType(t.item_type)
On 2012/06/19 20:33:10, kalman wrote:
> It looks like model.py normalises this and always sets properties/functions so
> you don't need to check with getattr.
> 
> (in fact needing to use getattr everywhere would be a failing of a model type)

Done.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:45: return None
On 2012/06/19 20:33:10, kalman wrote:
> hmmmm I'm not sure whether no-types should be normalised as a empty list or as
> None (ditto properties etc). I'd probably say empty-list is probably less to
> think about but it's much of a muchness. 

Done.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:48:
types_list.append(self._GenerateType(types[t]))
On 2012/06/19 20:33:10, kalman wrote:
> try to have names like "type_name" rather than "t".

Done.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:54: if len(c.params) !=
0:
On 2012/06/19 20:33:10, kalman wrote:
>  > 0

I just made the params empty if it doesn't have them.

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator.py:112: d['properties'] =
self._GenerateProperties(self._namespace.properties)
On 2012/06/19 20:33:10, kalman wrote:
> return {
>   'name': self._namespace.name,
>   'types': self._GenerateTypes(..)
>   ..
> }
> 
> ?

Done.

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

https://chromiumcodereview.appspot.com/10577022/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/dict_generator_test.py:28: 
On 2012/06/19 20:33:10, kalman wrote:
> extra \n ?

Done.

[not at google - send to devlin, 2012-06-20 18:11:53]

lgtm with comments. Let's get this submitted and start writing some templates
(woo).

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handlebar_dict_generator.py (right):

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:27:
_RemoveNoDocs(json)
Would prefer not to modify the input json here. Make a copy first?

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:53:
type_dict['item_type'] = self._GenerateType(type_.item_type)
It's this kind of stuff that I think will end up changing when we actually write
the template for rendering a type. I.e. we will certainly need to know what the
item type of an array is so that we can render "array of string" but I see this
more as having a separate dictionary called "array", like

if type_.type == ARRAY:
  type_dict['array'] = {
    'type': self._GenerateType(type_.item_type)
  }

and the templates will have like

{{?array}}
array of {{type.type}}
{{/}}

as opposed to

{{?item_type}}
array of {{type.type}}
{{/}}

A trivial example really, but anyway, that's just something to keep in mind.

So: how about just changing the key to "array" for now and seeing what happens.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:104:
property_dict['item_type'] = self._GenerateProperty(property_.item_type)
ditto, my comment above applies to all these "type == Foo" things. 'ref_type' ->
'ref', 'item_type' -> 'array'.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py
(right):

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py:20:
return f.read()
observation: a lot of tests seem to have this pattern. Consider having your own
test class that inherits from unittest.TestCase that can set up your testing
environment.

(not in this patch though)

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py:29:
self._GenerateTest('test_file.json')
why this indirection?

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

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:27: return
template.render(context, self).text
Why this change? Unless I'm misunderstanding, it means that any misspelled
variable name in the templates will fall through to here, which will grab stuff
from the fetcher.

I guess that's not a big deal provided we cache cache-misses too (do we?).

But I also just kinda like the fact that the sub-templates are namespaced. Avoid
potential problems with overriding references in the templates later on.

[cduvall, 2012-06-20 18:24:49]

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handlebar_dict_generator.py (right):

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:27:
_RemoveNoDocs(json)
On 2012/06/20 18:11:53, kalman wrote:
> Would prefer not to modify the input json here. Make a copy first?

Done.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:53:
type_dict['item_type'] = self._GenerateType(type_.item_type)
On 2012/06/20 18:11:53, kalman wrote:
> It's this kind of stuff that I think will end up changing when we actually
write
> the template for rendering a type. I.e. we will certainly need to know what
the
> item type of an array is so that we can render "array of string" but I see
this
> more as having a separate dictionary called "array", like
> 
> if type_.type == ARRAY:
>   type_dict['array'] = {
>     'type': self._GenerateType(type_.item_type)
>   }
> 
> and the templates will have like
> 
> {{?array}}
> array of {{type.type}}
> {{/}}
> 
> as opposed to
> 
> {{?item_type}}
> array of {{type.type}}
> {{/}}
> 
> A trivial example really, but anyway, that's just something to keep in mind.
> 
> So: how about just changing the key to "array" for now and seeing what
happens.

Done.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator.py:104:
property_dict['item_type'] = self._GenerateProperty(property_.item_type)
On 2012/06/20 18:11:53, kalman wrote:
> ditto, my comment above applies to all these "type == Foo" things. 'ref_type'
->
> 'ref', 'item_type' -> 'array'.

Done.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py
(right):

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py:20:
return f.read()
On 2012/06/20 18:11:53, kalman wrote:
> observation: a lot of tests seem to have this pattern. Consider having your
own
> test class that inherits from unittest.TestCase that can set up your testing
> environment.
> 
> (not in this patch though)

Good idea.

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/handlebar_dict_generator_test.py:29:
self._GenerateTest('test_file.json')
On 2012/06/20 18:11:53, kalman wrote:
> why this indirection?

I just wanted to make it easier to have similar tests that use different test
files.

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

http://codereview.chromium.org/10577022/diff/11/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/template_data_source.py:27: return
template.render(context, self).text
On 2012/06/20 18:11:53, kalman wrote:
> Why this change? Unless I'm misunderstanding, it means that any misspelled
> variable name in the templates will fall through to here, which will grab
stuff
> from the fetcher.
> 
> I guess that's not a big deal provided we cache cache-misses too (do we?).
> 
> But I also just kinda like the fact that the sub-templates are namespaced.
Avoid
> potential problems with overriding references in the templates later on.

Yeah, I changed this because I thought it would work better (and didn't
completely understand partials at the time), but  I changed it back when I was
messing with implementing a sample template.