Code generation for extensions api

tools/json_schema_compiler/model.py

+# 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 os.path
+
+class Model(object):
+  """Model of the API.
+  """
+  def __init__(self):
+    self.namespaces = {}
+    self.types = {}
+
+  def add_namespace(self, json, root_namespace, parent_path, filename_suffix):

[not at google - send to devlin, 2012/01/17 05:42:32]

parent_path is a confusing name, it's the source file right?  call it
source_file then.

also, I don't think that the model should know about the destination filename,
that's a concern of the generators, so "filename_suffix" (and "filename") don't
quite describe what these are.  it's more like the name of the namespace that's
being generated (which happens to almost be what files are being generated).  So
rather than "filename" how about "target_namespace", and instead of
"filename_suffix" like "target_namespace_suffix".

And for root_namespace, this argument would be unnecessary if it was part of the
global string substitution right?  I think that would be nicer rather than
needing to inject it here; it's not really a concern of the model (particularly
since it's the same for all namespaces), but rather an implementation detail of
code generation.

[calamity, 2012/01/18 05:43:08]

I'm a little iffy with global substitution because it's not clear where things
are defined. In particular, the root namespace is set once but has to be
substituted in every Code object. That's at odds with the current safety check
(don't add Code objects if there's anything unsubstituted). I could change the
whole generator to use one object but that Code object will become bigger and
clunkier. Alternatively I could remove the safety check and do the substitution
at Code.concat time.

+    """Add a namespace's json to the model.

[not at google - send to devlin, 2012/01/17 05:42:32]

.. "if it has a "compile" property set to true.
Returns the new namespace, or None if a namespace wasn't added.
"""

[calamity, 2012/01/18 05:43:08]

Done.

+    """
+    if not json.get('compile'):
+      return None
+    namespace =  Namespace(json, root_namespace,

[not at google - send to devlin, 2012/01/17 05:42:32]

extra space.

[calamity, 2012/01/18 05:43:08]

Done.

+        parent_path, self, filename_suffix)
+    self.namespaces[namespace.name] = namespace
+    for tipe in namespace.types.values():
+      self.types[tipe.name] = namespace
+    return namespace
+
+class Namespace(object):
+  """An API namespace.
+  """
+  def __init__(self, json, root_namespace,
+      parent_path, parent_model, filename_suffix):
+    self.name = json['namespace']
+    self.root_namespace = root_namespace
+    self.parent_path = parent_path
+    self.parent_dir, self.parent_filename = os.path.split(parent_path)

[not at google - send to devlin, 2012/01/17 05:42:32]

from what I can tell, this property is unnecessary; where you've used it you can
just as logically use parent_path (soon to be called "source_file").

and parent_filename isn't actually needed at all, I think.

+    self.parent_model = parent_model

[not at google - send to devlin, 2012/01/17 05:42:32]

what's parent_model used for?

[calamity, 2012/01/18 05:43:08]

Removed.

+    self.filename = self.name + filename_suffix
+    self.type_dependencies = {}
+    self.types = {}
+    self.functions = {}
+    for type_json in json['types']:
+      tipe = Type(type_json)
+      self.types[tipe.name] = tipe
+    for function_json in json['functions']:
+      if not function_json.get('nocompile'):
+        function = Function(function_json)
+        self.functions[function.name] = function
+
+class Type(object):
+  """A Type defined in the json.
+  """
+  def __init__(self, json):
+    self.name = json['id']
+    self.description = json.get('description')
+    self.properties = {}
+    for prop_name, prop_json in json['properties'].items():
+      self.properties[prop_name] = Property(prop_name, prop_json)
+
+class Callback(object):
+  """A callback parameter to a Function.
+  """
+  def __init__(self, json):
+    self.param = None
+    # Callbacks can have at most a single parameter
+    assert len(json['parameters']) <= 1
+    for param in json['parameters']:
+      self.param = Property(param['name'], param)

[not at google - send to devlin, 2012/01/17 05:42:32]

yeah using a loop looks a bit strage given the line/comment above.  How about
something along the lines of

parameters = json['parameters']
if len(parameters) == 0:
  self.param = None
elif len(parameters) == 1:
  param = parameters[0]
  self.param = Property(param.name, param)
else
  raise AssertionError("callbacks can have at most a single parameter")

(I think that an assertion message is probably more useful than a comment..
sorry about that)

[calamity, 2012/01/18 05:43:08]

Done.

+
+class Function(object):
+  """A Function defined in the API.
+  """
+  def __init__(self, json):
+    self.name = json['name']
+    self.params = []
+    self.description = json['description']
+    self.callback = None
+    self.type_dependencies = {}
+    for param in json['parameters']:
+      if param.get('type') == 'function':
+        self.callback = Callback(param)

[not at google - send to devlin, 2012/01/17 05:42:32]

assert not self.callback

[calamity, 2012/01/18 05:43:08]

Done.

+        continue
+      self.params.append(Property(param['name'], param))

[not at google - send to devlin, 2012/01/17 05:42:32]

just put in an else rather than a continue?

[calamity, 2012/01/18 05:43:08]

Done.

+    if not self.callback:
+      raise KeyError

[not at google - send to devlin, 2012/01/17 05:42:32]

i'm not sure using a KeyError is appropriate?  reading about it, it's thrown
when you look up a key in a dictionary which isn't there.  so it's not quite the
same thing.

I'd either assert here (with a message), or perhaps we should be defining our
own Exception somewhere and throwing those rather than asserting when there are
problems with the JSON (well, let's do this in a follow-up patch, if at all).

[calamity, 2012/01/18 05:43:08]

Agreed. Left this as an AssertionError for now.

+
+# TODO(calamity): handle Enum/choices
+class Property(object):
+  """A property of a type OR a parameter to a function.
+
+  Members will change based on PropertyType. Check self.type to determine which
+  members actually exist.
+  """
+  def __init__(self, name, json):
+    self.name = name
+    self.optional = json.get('optional', False)
+    self.description = json.get('description')
+    simple_types = ['boolean', 'integer', 'string', 'double']
+    # TODO(calamity) maybe check for circular refs? could that be a problem?
+    if '$ref' in json:
+      self.json_type = json['$ref']

[not at google - send to devlin, 2012/01/17 05:42:32]

the distinction between "json_type", "item_type", and "type" confuse me, mainly
that "json_type" is a pretty ambiguous name.

I think the following would be clearer:

Have PropertyType enumerate all the types; don't generalise a few under
FUNDAMENTAL:

class PropertyType(object):
  BOOLEAN, INTEGER, STRING, DOUBLE, ARRAY, REF, CHOICES, OBJECT = range(8)

in the case of what you're calling a "simple type", don't set an extra property
at all; the type is entirely defined by the value of type.

in the case of ARRAY and OBJECT, do what you're already doing.

(then update the comment accordingly.)

[calamity, 2012/01/18 05:43:08]

Agreed. That's what the comment I made last revision was saying.

+      self.type = PropertyType.REF
+    elif 'type' in json:
+      json_type = json['type']
+      if json_type in simple_types:
+        self.json_type = json_type
+        self.type = PropertyType.FUNDAMENTAL
+      elif json_type == 'array':
+        self.item_type = Property(name + "_inner", json['items'])
+        self.type = PropertyType.ARRAY
+      elif json_type == 'object':
+        self.properties = {}
+        self.type = PropertyType.OBJECT
+        for key, val in json['properties'].items():
+          self.properties[key] = Property(key, val)
+      else:
+        raise NotImplementedError(json_type)
+    elif 'choices' in json:
+      self.type = PropertyType.CHOICES
+      self.choices = {}
+
+class PropertyType(object):
+  """Enum of different types of properties/parameters.
+  """
+  STRING, FUNDAMENTAL, ARRAY, REF, CHOICES, OBJECT = range(6)