Extensions Docs Server: Generalize $ref's to work for any schema node

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

Index: chrome/common/extensions/docs/server2/docs_server_utils.py
diff --git a/chrome/common/extensions/docs/server2/docs_server_utils.py b/chrome/common/extensions/docs/server2/docs_server_utils.py
index 621e46fac29b001890ea81ce40d9cf64482b2988..1fbacde1b312537bd299b8cc0f989bd7576c6428 100644
--- a/chrome/common/extensions/docs/server2/docs_server_utils.py
+++ b/chrome/common/extensions/docs/server2/docs_server_utils.py
@@ -4,14 +4,16 @@
 
 import os
 
+from file_system import FileNotFoundError
+
 def FormatKey(key):
   """Normalize a key by making sure it has a .html extension, and convert any
   '.'s to '_'s.
   """
   if key.endswith('.html'):
-    key = key[:-5]
+    key = key[:-len('.html')]
   safe_key = key.replace('.', '_')
-  return safe_key + '.html'
+  return '%s.html' % safe_key
 
 def SanitizeAPIName(name, api_path=''):
   """Sanitizes API filenames that are in subdirectories.
@@ -21,13 +23,56 @@ def SanitizeAPIName(name, api_path=''):
     filename = 'experimental_' + filename.replace('experimental_', '')
   return filename
 
-def GetLinkToRefType(namespace_name, ref_type):
+def _FindInAPI(node_name, api):

[not at google - send to devlin, 2012/11/01 22:00:49]

Find what in API? And what does it return? I was surprised to see that this
returns a string, not the node itself. It makes sense when you look at the
callers, but perhaps a name along the lines of "FindSchemaGroupName" or
"ClassifySchemaNode". "Type" would be appropriate but it's overloaded.

[cduvall, 2012/11/02 01:53:53]

Done.

+  node_name = node_name.split('.')[0]
+  for type_ in api.get('types', []):
+    if type_['name'] == node_name:

[not at google - send to devlin, 2012/11/01 22:00:49]

except the property is "id".

[cduvall, 2012/11/02 01:53:53]

This has been processed by JSCModel, so all nodes have the 'name' property.

[not at google - send to devlin, 2012/11/02 17:26:47]

Ah, of course.

+      return 'type'
+  for function in api.get('functions', []):
+    if function['name'] == node_name:
+      return 'method'
+  for event in api.get('events', []):
+    if event['name'] == node_name:

[not at google - send to devlin, 2012/11/01 22:00:49]

how about

for name in ['functions', 'events']:
  for item in api.get(name, []):
    ...

[cduvall, 2012/11/02 01:53:53]

Done.

+      return 'event'
+  for property_ in api.get('properties', []):
+    if property_['name'] == node_name:
+      return 'property'

[not at google - send to devlin, 2012/11/01 22:00:49]

... except "properties" is a dictionary not a list. We might also want to check
it first, since it's cheapest to look up a dictionary and it looks like the most
commonly referenced thing is runtime.lastError.

[cduvall, 2012/11/02 01:53:53]

This has been processed by JSCModel, so it's a list.

+  return None

[not at google - send to devlin, 2012/11/01 22:00:49]

And also, we need to search deeply through nodes. I see references to things
like "method-StorageArea-clear", and this wouldn't find it.

I'd actually just suggest doing a more generic search through the dictionary,
but I think that would show up false positives. So what you have is fine.

[not at google - send to devlin, 2012/11/01 22:05:15]

This part of the comment was made before I read past the bottom. Never mind, I
can see it's handled below.

[cduvall, 2012/11/02 01:53:53]

I made this more robust so it checks to make sure things like
"method-StorageArea-clear" actually exist.

+
+def GetLinkToRefType(namespace_name, ref_type, api_data_source, api_list):

[not at google - send to devlin, 2012/11/01 22:00:49]

You're gonna hate me here but now that there's injected dependencies consider
pulling this out into a class like "ReferenceResolver".

[cduvall, 2012/11/02 01:53:53]

Done.

   """Returns a link given a $ref.
   """
-  if ref_type.startswith(namespace_name + '.'):
-    type_name = ref_type[len(namespace_name + '.'):]
-    return { 'href': '#type-' + type_name, 'text': type_name }
+  if ref_type.startswith('%s.' % namespace_name):
+    name = ref_type[len('%s.' % namespace_name):]
+    try:
+      type_name = _FindInAPI(name, api_data_source[namespace_name])

[not at google - send to devlin, 2012/11/01 22:00:49]

as I mentioned above, "type" is overloaded in this context and it's rather
confusing. I suggested "group" above but it's not the best name, maybe you can
think of something better.

[cduvall, 2012/11/02 01:53:53]

Done.

+    except FileNotFoundError:

[not at google - send to devlin, 2012/11/01 22:05:15]

But speaking of that, this seems like a bug where .s in the names aren't being
handled.

[cduvall, 2012/11/02 01:53:53]

I tried this with refs to app.window.create and links to
storage.StorageArea.get, and it worked for both.

[not at google - send to devlin, 2012/11/02 17:26:47]

I meant like, if there's a reference to 'app.window.create' from the 'app' docs,
I don't think this will work? I've mentioned it in the comments for the latest
patch.

+      return None
+    if type_name is None:
+      return None
+    return { 'href': '#%s-%s' % (type_name, name), 'text': name }
   elif '.' not in ref_type:
-    return { 'href': '#type-' + ref_type, 'text': ref_type }
-  api, type_name = ref_type.rsplit('.', 1)
-  return { 'href': api + '.html#type-' + type_name, 'text': ref_type }
+    try:
+      type_name = _FindInAPI(ref_type, api_data_source[namespace_name])
+    except FileNotFoundError:
+      return None
+    if type_name is None:
+      return None
+    return { 'href': '#%s-%s' % (type_name, ref_type), 'text': ref_type }

[not at google - send to devlin, 2012/11/01 22:00:49]

It seems like the above two conditions could be combined by just stripping the
namespace name if it's there.

if ref_type.startwith('%s.' % namespace_name):
  name = ...
else:
  name = ref_type

?

[cduvall, 2012/11/02 01:53:53]

Done.

+  parts = ref_type.split('.')
+  for i, part in enumerate(parts):
+    if '.'.join(parts[:i]) not in api_list:
+      continue
+    try:
+      api = api_data_source['.'.join(parts[:i])]

[not at google - send to devlin, 2012/11/01 22:00:49]

'.'.join(parts[:i]) is repeated, assign to variable api_name?

[cduvall, 2012/11/02 01:53:53]

Done.

+    except FileNotFoundError:
+      continue
+    name = '.'.join(parts[i:])
+    type_name = _FindInAPI(name, api)

[not at google - send to devlin, 2012/11/01 22:00:49]

again, s/type_name/?/

[cduvall, 2012/11/02 01:53:53]

Done.

+    if type_name is None:
+      continue
+    return {
+      'href': '%s.html#%s-%s' % ('.'.join(parts[:i]), type_name, name),

[not at google - send to devlin, 2012/11/01 22:00:49]

re-use api_name here.

[cduvall, 2012/11/02 01:53:53]

Done.

+      'text': ref_type
+    }
+  return None