Automatically detect inline-able doc in IDL files

Automatically detect inline-able doc in IDL files

Apply the inline_doc tag to IDL entries that are only referenced once so that they are rendered inline.

BUG=239193
R=kalman@chromium.org

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

[jshumway, 2013-05-14 05:14:22]

Here is the initial version. There is one thing that still needs to be worked
out; there are about ten or so links that are broken when documents become
inlined.

I rewrote the recursive call to explicitly use a stack as part of debugging
(that call turned out not to be the issue) but the explicit one looked a little
nicer so I left it in. If I should prefer the actual recursive one, I can swap
them (not sure what the preference is).

[not at google - send to devlin, 2013-05-14 05:37:56]

looks good, what are the failures?

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:49: if
node.get('$ref'):
'$ref' in node?

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/app.yaml:3: runtime: python27
presubmit message needs to be better, this should be 2-3-0.

[not at google - send to devlin, 2013-05-14 06:12:25]

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:38: banned =
frozenset(('returns', 'value', 'items'))
as discussed - let's remove 'returns' and 'items' from here, then fix the bugs
that are causing those not to render:
- not fully rendering types of properties
- neither fully return types
- nor even displaying it for IDL (this might be a bug in idl_schema.py)

I'm sure Clark can help you find your way around the templates.

then, don't inline enum types, and make those changes in the alarms API to
convert the $ref:delayInMinutes to just <var>delayInMinutes</var> and we're
good.

[jshumway, 2013-05-15 03:31:59]

Excluded enums, returns are inlined, and the IDL files have been changed.

[not at google - send to devlin, 2013-05-15 03:55:15]

Cool. By "no way to inline items" do you mean to display them properly? Yeah, we
can fix that later. This is a great start.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/alarms.idl:54: // more.  That is, setting
<code><var>delayInMinutes</var></code> or
Either <var> or <code>, not both; which looks better?

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:31: def
_DetectInlineableIdl(schema):
_DetectInlinableTypes?

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:38: banned =
frozenset(('enum', 'value', 'choices', 'items'))
banned sounds so mean. maybe 'exclude'.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:40: # Use an explicit
stack instead of recursion.
comment not really necessary

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:48: if
node.get('$ref'):
'$ref' in node

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:52: stack.append(v)
stack.append(v for k, v in node.iteritems() if k in not banned)

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:54: for doc in
schema['types']:
for type_ in ...

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:74: del
type_['inline_doc']
this doesn't look right

that said, checking for description is unnecessary too. these can be

for k in ('id', 'description', 'inline_doc'):
  type_.pop(k, None)

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-3-1
2-3-0. I will fix that presubmit sometime, sorry...

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/function.html (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/function.html:24:
{{+partials.type api:api type:returns anonymous_return:true}}
see other comment. you'd leave out display_name here.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/type.html (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/type.html:6:
{{/anonymous_return}}
Move the Returns into the template which includes this, and below comment,
rather than using the anonymous_return flag.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/type.html:20:
<h4>Properites</h4>
Well this should be Properties...

but anyway, a better model is probably to pass the name into this template
explicitly. It's only used once. That would let you do like

{{?display_name}}<h3 id="{{type.id}}">{{display_name}}</h3>

...

Properties{{?display_name}} of <a
href="#type={{display_name}}>{{display_name}}</a>{{/}}

etc. The methods/functions need the same treatment.

[jshumway, 2013-05-15 04:51:37]

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/alarms.idl (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/alarms.idl:54: // more.  That is, setting
<code><var>delayInMinutes</var></code> or
On 2013/05/15 03:55:15, kalman wrote:
> Either <var> or <code>, not both; which looks better?

I made it code to be consistent with the last IDL patch I made.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:31: def
_DetectInlineableIdl(schema):
On 2013/05/15 03:55:15, kalman wrote:
> _DetectInlinableTypes?

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:38: banned =
frozenset(('enum', 'value', 'choices', 'items'))
On 2013/05/15 03:55:15, kalman wrote:
> banned sounds so mean. maybe 'exclude'.

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:40: # Use an explicit
stack instead of recursion.
On 2013/05/15 03:55:15, kalman wrote:
> comment not really necessary

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:48: if
node.get('$ref'):
On 2013/05/15 03:55:15, kalman wrote:
> '$ref' in node

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:52: stack.append(v)
On 2013/05/15 03:55:15, kalman wrote:
> stack.append(v for k, v in node.iteritems() if k in not banned)

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:54: for doc in
schema['types']:
On 2013/05/15 03:55:15, kalman wrote:
> for type_ in ...

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:74: del
type_['inline_doc']
On 2013/05/15 03:55:15, kalman wrote:
> this doesn't look right
> 
> that said, checking for description is unnecessary too. these can be
> 
> for k in ('id', 'description', 'inline_doc'):
>   type_.pop(k, None)

Opps, that was definitely a misedit.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-3-1
On 2013/05/15 03:55:15, kalman wrote:
> 2-3-0. I will fix that presubmit sometime, sorry...

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/function.html (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/function.html:24:
{{+partials.type api:api type:returns anonymous_return:true}}
On 2013/05/15 03:55:15, kalman wrote:
> see other comment. you'd leave out display_name here.

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/type.html (right):

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/type.html:6:
{{/anonymous_return}}
On 2013/05/15 03:55:15, kalman wrote:
> Move the Returns into the template which includes this, and below comment,
> rather than using the anonymous_return flag.

Done.

https://codereview.chromium.org/15127007/diff/7001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/type.html:20:
<h4>Properites</h4>
On 2013/05/15 03:55:15, kalman wrote:
> Well this should be Properties...
> 
> but anyway, a better model is probably to pass the name into this template
> explicitly. It's only used once. That would let you do like
> 
> {{?display_name}}<h3 id="{{type.id}}">{{display_name}}</h3>
> 
> ...
> 
> Properties{{?display_name}} of <a
> href="#type={{display_name}}>{{display_name}}</a>{{/}}
> 
> etc. The methods/functions need the same treatment.

Sorry, was typing too quickly

[not at google - send to devlin, 2013-05-15 05:13:31]

lgtm, just as discussed in irc. also it would be nice if this were tested...

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:70: for k in
['description', 'id', 'inline_doc']:
tuple not list, it's immutable

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/type.html (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/type.html:24: Methods of <a
href="#type-{{type.name}}">{{type.name}}</a>
this needs the display_name check too

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/type.html:32: Events of <a
href="#type-{{type.name}}">{{type.name}}</a>
ditto

[not at google - send to devlin, 2013-05-15 05:18:01]

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/type.html (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/type.html:13: Properties of <a
href="#type-{{type.name}}"></a>
also this is a typo, lost the name. Just do s/type.name/display_name/ in here
and add it back in the <a> content.

[not at google - send to devlin, 2013-05-15 05:19:18]

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:38: banned =
frozenset(('enum', 'value', 'choices', 'items'))
also I'm still seeing enums being inlined, see notifications.html

[jshumway, 2013-05-15 05:55:45]

Reverted the returns stuff including the html template changes. Tracked down and
fixed the enum inline bug.

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:38: banned =
frozenset(('enum', 'value', 'choices', 'items'))
On 2013/05/15 05:19:18, kalman wrote:
> also I'm still seeing enums being inlined, see notifications.html

Done.

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:70: for k in
['description', 'id', 'inline_doc']:
On 2013/05/15 05:13:31, kalman wrote:
> tuple not list, it's immutable

Okay, I will catch this next time

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/private/type.html (right):

https://codereview.chromium.org/15127007/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/private/type.html:13: Properties of <a
href="#type-{{type.name}}"></a>
On 2013/05/15 05:18:01, kalman wrote:
> also this is a typo, lost the name. Just do s/type.name/display_name/ in here
> and add it back in the <a> content.

I ended up reverting all of this as we discussed

[not at google - send to devlin, 2013-05-15 06:31:20]

Committed patchset #5 manually as r200182.