Issue 9114036: Code generation for extensions api

not at google - send to devlin

Sorry, didn't nearly get through this, just a few nitpicks and a medium-level comments. Sending ...

8 years, 11 months ago (2012-01-12 06:01:05 UTC) #1

calamity

http://codereview.chromium.org/9114036/diff/2001/build/api_gen.gypi File build/api_gen.gypi (right): http://codereview.chromium.org/9114036/diff/2001/build/api_gen.gypi#newcode7 build/api_gen.gypi:7: 'api_gen_dir': '<(DEPTH)/tools/json_schema_compiler', On 2012/01/12 06:01:05, kalman wrote: > you'll ...

8 years, 11 months ago (2012-01-12 22:59:20 UTC) #2

not at google - send to devlin

Replying to your questions from that last review; plus a couple of extra things. I'll ...

8 years, 11 months ago (2012-01-13 00:07:27 UTC) #3

Replying to your questions from that last review; plus a couple of extra things.

I'll continue looking at this in more detail now.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:43: """Concatenate another Code object onto
this one.
call it "concat" then?  add is an odd name.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:57: def sblock(self, line,
indent=INDENT_SIZE):
On 2012/01/12 22:59:20, calamity wrote:
> Can you explain what you dislike about the current API?
> 
> If it's indenting, I've only recently discovered that I've been a total noob
and
> you can just go
> (c.append("foo")
>   .sblock("bar1")
>     .append("bar2")
>     .append("bar3")
>   .eblock()
>   .append("baz")
> )
> 
> The reason I didn't do this initially is that I didn't know that the python
> parser would fail without the enclosing parentheses. Pretty lame.

Oh cool, I didn't know that either.  I was assuming you had to use \ everywhere.

Anyway -- I just find the use of sblock/eblock a bit confusing, particularly
passing it a line. Do you really need that?  How about something like

(c.append('foo')
  .append('bar1').start_block()
    .append('bar2')
    .append('bar3').end_block()
  .append('baz'))

> 
> 
> On 2012/01/12 06:01:05, kalman wrote:
> > I don't like the names "sblock" and "eblock" much anyway. How about a
slightly
> > different API for Code in the first place:
> 
> I put those names because they worked out to 6 characters which is the same as
> append

Cool, you can avoid it if you take my suggestion above.

> 
> > 
> > Have a block() method, which returns a new, indented Code object that you
can
> > append to. In other words, code like this:
> > 
> > c.append("foo")
> > c.sblock("bar1")
> > c.append("bar2")
> > c.append("bar3")
> > c.eblock()
> > c.append("baz")
> > 
> > would become like this:
> > 
> > c.append("foo")
> > c.block() \
> >   .append("bar1") \
> >   .append("bar2") \
> >   .append("bar3")
> > c.append("baz")
> > 
> > or
> > 
> > c.append("foo")
> > indented = c.block()
> > indented.append("bar1")
> > indented.append("bar2")
> > indented.append("bar3")
> > c.append("bar")
> > 
> 
> > likewise, rather than add()ing a Code to the code object, you'd get a new
one,
> > like "section()" or something -- which I believe you only need to be able to
> > substitute names across sections of code, right?
> > 
> > You'd then factor the code generation to take the Code to generate into,
> rather
> > than creating a new one internally if needed.  I.e. rather than
> > 
> > def generate_some_code(self):
> >   c = Code()
> >   ...
> >   return c
> > 
> > c.add(generate_some_code())
> > 
> > you'd do
> > 
> > def generate_some_code(self, c):
> >   ...
> > 
> > generate_some_code(c.section())
> 
> Considered passing the same code object everywhere, looked messy because
> functions have a fair number of params already.

Heh, well parameter arguments are cheap.  I wouldn't worry about that.

But yeah, I'm just being an API astronaut*; I think that what I wrote up there
is neat, but after sleeping, don't worry about it; it's too much work to go and
rewrite stuff.

* as opposed to an architecture astronaut.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:99: """Goes through each line and
interpolates using the given dict.
Explain why you need to have this method at all, rather than just using normal
string interpolation everywhere.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/g...
File tools/json_schema_compiler/generate_cc.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/g...
tools/json_schema_compiler/generate_cc.py:28: #  c.append('#include
"base/json/json_converter.h"')
On 2012/01/12 22:59:20, calamity wrote:
> On 2012/01/12 06:01:05, kalman wrote:
> > what's this?
> 
> Don't know, it was in the original spec. You said we wouldn't need it "for
now"
> so I left it commented out. Maybe it was the original name for utils?
> 
> Removed.

Oh yeah, never mind about that.  Sorry.

not at google - send to devlin

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py File tools/json_schema_compiler/code.py (right): http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py#newcode57 tools/json_schema_compiler/code.py:57: def sblock(self, line, indent=INDENT_SIZE): On 2012/01/13 00:07:27, kalman wrote: ...

8 years, 11 months ago (2012-01-13 00:09:41 UTC) #4

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:57: def sblock(self, line,
indent=INDENT_SIZE):
On 2012/01/13 00:07:27, kalman wrote:
> On 2012/01/12 22:59:20, calamity wrote:
> > Can you explain what you dislike about the current API?
> > 
> > If it's indenting, I've only recently discovered that I've been a total noob
> and
> > you can just go
> > (c.append("foo")
> >   .sblock("bar1")
> >     .append("bar2")
> >     .append("bar3")
> >   .eblock()
> >   .append("baz")
> > )
> > 
> > The reason I didn't do this initially is that I didn't know that the python
> > parser would fail without the enclosing parentheses. Pretty lame.
> 
> Oh cool, I didn't know that either.  I was assuming you had to use \
everywhere.
> 
> Anyway -- I just find the use of sblock/eblock a bit confusing, particularly
> passing it a line. Do you really need that?  How about something like
> 
> (c.append('foo')
>   .append('bar1').start_block()
>     .append('bar2')
>     .append('bar3').end_block()
>   .append('baz'))
> 
> > 
> > 
> > On 2012/01/12 06:01:05, kalman wrote:
> > > I don't like the names "sblock" and "eblock" much anyway. How about a
> slightly
> > > different API for Code in the first place:
> > 
> > I put those names because they worked out to 6 characters which is the same
as
> > append
> 
> Cool, you can avoid it if you take my suggestion above.
> 
> > 
> > > 
> > > Have a block() method, which returns a new, indented Code object that you
> can
> > > append to. In other words, code like this:
> > > 
> > > c.append("foo")
> > > c.sblock("bar1")
> > > c.append("bar2")
> > > c.append("bar3")
> > > c.eblock()
> > > c.append("baz")
> > > 
> > > would become like this:
> > > 
> > > c.append("foo")
> > > c.block() \
> > >   .append("bar1") \
> > >   .append("bar2") \
> > >   .append("bar3")
> > > c.append("baz")
> > > 
> > > or
> > > 
> > > c.append("foo")
> > > indented = c.block()
> > > indented.append("bar1")
> > > indented.append("bar2")
> > > indented.append("bar3")
> > > c.append("bar")
> > > 
> > 
> > > likewise, rather than add()ing a Code to the code object, you'd get a new
> one,
> > > like "section()" or something -- which I believe you only need to be able
to
> > > substitute names across sections of code, right?
> > > 
> > > You'd then factor the code generation to take the Code to generate into,
> > rather
> > > than creating a new one internally if needed.  I.e. rather than
> > > 
> > > def generate_some_code(self):
> > >   c = Code()
> > >   ...
> > >   return c
> > > 
> > > c.add(generate_some_code())
> > > 
> > > you'd do
> > > 
> > > def generate_some_code(self, c):
> > >   ...
> > > 
> > > generate_some_code(c.section())
> > 
> > Considered passing the same code object everywhere, looked messy because
> > functions have a fair number of params already.
> 
> Heh, well parameter arguments are cheap.  I wouldn't worry about that.
> 
> But yeah, I'm just being an API astronaut*; I think that what I wrote up there
> is neat, but after sleeping, don't worry about it; it's too much work to go
and
> rewrite stuff.
> 
> 
> * as opposed to an architecture astronaut.

Or similarly,

(c.append('foo')
  .append('bar1').indent()
    .append('bar2')
    .append('bar3').unindent()
  .append('baz'))

hah.  I prefer the aesthetic of the former (start_block/end_block) but
indent/unindent might be more accurate.  The "hah" is because you had this a
while ago.

calamity

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py File tools/json_schema_compiler/code.py (right): http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py#newcode57 tools/json_schema_compiler/code.py:57: def sblock(self, line, indent=INDENT_SIZE): On 2012/01/13 00:07:27, kalman wrote: ...

8 years, 11 months ago (2012-01-13 00:40:22 UTC) #5

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:57: def sblock(self, line,
indent=INDENT_SIZE):
On 2012/01/13 00:07:27, kalman wrote:
> On 2012/01/12 22:59:20, calamity wrote:
> > Can you explain what you dislike about the current API?
> > 
> > If it's indenting, I've only recently discovered that I've been a total noob
> and
> > you can just go
> > (c.append("foo")
> >   .sblock("bar1")
> >     .append("bar2")
> >     .append("bar3")
> >   .eblock()
> >   .append("baz")
> > )
> > 
> > The reason I didn't do this initially is that I didn't know that the python
> > parser would fail without the enclosing parentheses. Pretty lame.
> 
> Oh cool, I didn't know that either.  I was assuming you had to use \
everywhere.
> 
> Anyway -- I just find the use of sblock/eblock a bit confusing, particularly
> passing it a line. Do you really need that?  How about something like
> 
> (c.append('foo')
>   .append('bar1').start_block()
>     .append('bar2')
>     .append('bar3').end_block()
>   .append('baz'))
> 

It was more error proof. Calling unindent _after_ the append is problems.

Especially since the reversal is necessary. You wouldn't actually call
.append.end_block(), you'd call .end_block().append() when there are conditions
so the end_block is always run.

 (c.append('foo')
   .append('bar1').start_block()
 )
   if condition1:
     (c.append('bar2')
       .append('bar3')
     )
   else:
     (c.append('bar4')
       .append('bar5')
     )      
 c.end_block().append('baz'))
   ^^^^^^^^^^^^^^^^^^
       Fail bait.

> > 
> > 
> > On 2012/01/12 06:01:05, kalman wrote:
> > > I don't like the names "sblock" and "eblock" much anyway. How about a
> slightly
> > > different API for Code in the first place:
> > 
> > I put those names because they worked out to 6 characters which is the same
as
> > append
> 
> Cool, you can avoid it if you take my suggestion above.
> 
> > 
> > > 
> > > Have a block() method, which returns a new, indented Code object that you
> can
> > > append to. In other words, code like this:
> > > 
> > > c.append("foo")
> > > c.sblock("bar1")
> > > c.append("bar2")
> > > c.append("bar3")
> > > c.eblock()
> > > c.append("baz")
> > > 
> > > would become like this:
> > > 
> > > c.append("foo")
> > > c.block() \
> > >   .append("bar1") \
> > >   .append("bar2") \
> > >   .append("bar3")
> > > c.append("baz")
> > > 
> > > or
> > > 
> > > c.append("foo")
> > > indented = c.block()
> > > indented.append("bar1")
> > > indented.append("bar2")
> > > indented.append("bar3")
> > > c.append("bar")
> > > 
> > 
> > > likewise, rather than add()ing a Code to the code object, you'd get a new
> one,
> > > like "section()" or something -- which I believe you only need to be able
to
> > > substitute names across sections of code, right?
> > > 
> > > You'd then factor the code generation to take the Code to generate into,
> > rather
> > > than creating a new one internally if needed.  I.e. rather than
> > > 
> > > def generate_some_code(self):
> > >   c = Code()
> > >   ...
> > >   return c
> > > 
> > > c.add(generate_some_code())
> > > 
> > > you'd do
> > > 
> > > def generate_some_code(self, c):
> > >   ...
> > > 
> > > generate_some_code(c.section())
> > 
> > Considered passing the same code object everywhere, looked messy because
> > functions have a fair number of params already.
> 
> Heh, well parameter arguments are cheap.  I wouldn't worry about that.
> 
> But yeah, I'm just being an API astronaut*; I think that what I wrote up there
> is neat, but after sleeping, don't worry about it; it's too much work to go
and
> rewrite stuff.
> 
> 
> * as opposed to an architecture astronaut.

I'm also more partial to a merge-y approach because each function is more
encapsulated and it becomes very obvious where things mess up. Otherwise for the
same safety I'd need checks to enforce code segments not touching each other
inside the Code class.

not at google - send to devlin

Next round. Haven't looked at the type checker, nor the tests. Also haven't gone through ...

8 years, 11 months ago (2012-01-13 02:14:09 UTC) #6

Next round.

Haven't looked at the type checker, nor the tests.

Also haven't gone through the C/H generators too closely though I think a bunch
of the comments I made in the other files will apply to them too.

I'm going to have lunch; ping me when you've gone through these.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:57: def sblock(self, line,
indent=INDENT_SIZE):
On 2012/01/13 00:40:23, calamity wrote:
> On 2012/01/13 00:07:27, kalman wrote:
> > On 2012/01/12 22:59:20, calamity wrote:
> > > Can you explain what you dislike about the current API?
> > > 
> > > If it's indenting, I've only recently discovered that I've been a total
noob
> > and
> > > you can just go
> > > (c.append("foo")
> > >   .sblock("bar1")
> > >     .append("bar2")
> > >     .append("bar3")
> > >   .eblock()
> > >   .append("baz")
> > > )
> > > 
> > > The reason I didn't do this initially is that I didn't know that the
python
> > > parser would fail without the enclosing parentheses. Pretty lame.
> > 
> > Oh cool, I didn't know that either.  I was assuming you had to use \
> everywhere.
> > 
> > Anyway -- I just find the use of sblock/eblock a bit confusing, particularly
> > passing it a line. Do you really need that?  How about something like
> > 
> > (c.append('foo')
> >   .append('bar1').start_block()
> >     .append('bar2')
> >     .append('bar3').end_block()
> >   .append('baz'))
> > 
> 
> It was more error proof. Calling unindent _after_ the append is problems.
> 
> Especially since the reversal is necessary. You wouldn't actually call
> .append.end_block(), you'd call .end_block().append() when there are
conditions
> so the end_block is always run.
> 
>  (c.append('foo')
>    .append('bar1').start_block()
>  )
>    if condition1:
>      (c.append('bar2')
>        .append('bar3')
>      )
>    else:
>      (c.append('bar4')
>        .append('bar5')
>      )      
>  c.end_block().append('baz'))
>    ^^^^^^^^^^^^^^^^^^
>        Fail bait.
> 

I don't understand what you mean... surely it's pretty clear what (un)indent
would do?

Never mind, though.

> > > 
> > > 
> > > On 2012/01/12 06:01:05, kalman wrote:
> > > > I don't like the names "sblock" and "eblock" much anyway. How about a
> > slightly
> > > > different API for Code in the first place:
> > > 
> > > I put those names because they worked out to 6 characters which is the
same
> as
> > > append
> > 
> > Cool, you can avoid it if you take my suggestion above.
> > 
> > > 
> > > > 
> > > > Have a block() method, which returns a new, indented Code object that
you
> > can
> > > > append to. In other words, code like this:
> > > > 
> > > > c.append("foo")
> > > > c.sblock("bar1")
> > > > c.append("bar2")
> > > > c.append("bar3")
> > > > c.eblock()
> > > > c.append("baz")
> > > > 
> > > > would become like this:
> > > > 
> > > > c.append("foo")
> > > > c.block() \
> > > >   .append("bar1") \
> > > >   .append("bar2") \
> > > >   .append("bar3")
> > > > c.append("baz")
> > > > 
> > > > or
> > > > 
> > > > c.append("foo")
> > > > indented = c.block()
> > > > indented.append("bar1")
> > > > indented.append("bar2")
> > > > indented.append("bar3")
> > > > c.append("bar")
> > > > 
> > > 
> > > > likewise, rather than add()ing a Code to the code object, you'd get a
new
> > one,
> > > > like "section()" or something -- which I believe you only need to be
able
> to
> > > > substitute names across sections of code, right?
> > > > 
> > > > You'd then factor the code generation to take the Code to generate into,
> > > rather
> > > > than creating a new one internally if needed.  I.e. rather than
> > > > 
> > > > def generate_some_code(self):
> > > >   c = Code()
> > > >   ...
> > > >   return c
> > > > 
> > > > c.add(generate_some_code())
> > > > 
> > > > you'd do
> > > > 
> > > > def generate_some_code(self, c):
> > > >   ...
> > > > 
> > > > generate_some_code(c.section())
> > > 
> > > Considered passing the same code object everywhere, looked messy because
> > > functions have a fair number of params already.
> > 
> > Heh, well parameter arguments are cheap.  I wouldn't worry about that.
> > 
> > But yeah, I'm just being an API astronaut*; I think that what I wrote up
there
> > is neat, but after sleeping, don't worry about it; it's too much work to go
> and
> > rewrite stuff.
> > 
> > 
> > * as opposed to an architecture astronaut.
> 
> I'm also more partial to a merge-y approach because each function is more
> encapsulated and it becomes very obvious where things mess up. Otherwise for
the
> same safety I'd need checks to enforce code segments not touching each other
> inside the Code class.

Fair enough.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:20: // DO NOT EDIT."""
This is a specific to C/C++, so code.py doesn't seem like the right place for
it.  How about having a shared file for c++ code generation which both
generate_cc and generate_h can use.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:23: COMMENT_LENGTH = 80
put as members / arguments in the constructor of Code, with default values set
to these.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:24: class Code(object):
is explicitly extending object necessary?

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:25: """A convenience object for appending
code.
s/appending/constructing/

or something.

Also add that all methods (besides |render|) return self.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:27: Logically each object should be a block
of code."""
these docstrings should have a blank line after them, and the closing """ should
be on a line of its own.

(same for every class)

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:30: # TODO(calamity): Indent stack probably
isn't necessary
you can delete this comment and _indent_list, right?

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:32: self.indent_level = 0
these should have an _ prefix

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:38: This will strip trailing whitespace."""
... newline if line is not specified. Trailing whitespace is stripped.
"""

more concise documentation ftw.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:42: def add(self, obj):
I wonder if it's more convenient to just merge this into append.  That would let
functions return either a string or a code block depending on what they were
feeling like, and it kind of looks nicer IMO.

def append(self, content=''):
  if isinstance(content, str):
    self.code.append(((' ' * self.indent_level)) + line).rstrip()
  elif isinstance(content, Code):
    for line in content.code:
      self.append(line)
  else:
    raise TypeError('blah type not supported blah')
  return self

you'd need to carefully document that too, of course.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:56: # TODO(calamity): is variable indent size
necessary/a good idea?
TODO unnecessary

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:58: """Starts a code block.
"""Starts a code block, by appending a line of code then increasing the indent
level.
"""

by the way, for consistency, this method should probably have line='' too.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:66: """Ends a code block.
"""Ends a code block, by decreasing the indent level then appending a line (or
a blank line if none is given.
"""

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:100: Raises type error if passed something
that isn't a dict."""
"Raises type error" comment is unnecessary.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:113: """Returns the code joined together as a
string."""
"""Renders the Code as a string.
"""

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:116: def cpp_name(s):
this also belongs in a c++ util class

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/generate_cc.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:9: class CC_Generator(object):
I like this name, so the file should be called cc_generator.py.

Also I believe that the style is just "class CCGenerator:" ...

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:17: """Generates the .cc code for a
single namespace.
"""Generates a code.Code object with the .cc code for a namespace.
"""

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:24:
(c.append(code.CHROMIUM_LICENSE).append()
put the second .append() on its own line; this would mirror the generated code
more closely.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:29: #    .append('#include
"base/json/json_converter.h"')
delete

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:31: .append('namespace extensions {')
"extensions" needs to be taken from the command-line options.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:40: c.add(self.generate_type(tipe))
pull the last "c.append()" out of generate_type and into here.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:47: c.append('} // namespace
extensions')
ditto extensions

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:53: """Generates the function
definitions for a type in the namespace."""
... for a type.
"""

(no need for "in the namespace")

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:55: classname = '%s' % (tipe.name)
can delete this line and inline it?

...
c.substitute({'classname': tipe.name})

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:88: c.append('if
(!json_schema_compiler::util::GetOptionalTypes<%(type)s>(*dict,')
this line is too long.

this makes me think though -- I think it would be nice to have a few global
variables that are automatically resolved at the top level (although probably by
the Code object, since the Code object needs to know about any global
substitutions so that unresolved ones can be error checked, anyway)

Things like
 - the path to the utils ('json_schema_compiler::util' in this case)
 - the namepace that everything is being generated to ('extensions' in this
case)

this line could then become

c.append("if (!%(global.utils)s::GetOptionalTypes(...)")

likewise where you've written "extensions", could be %(global.namespace)s or
whatever you've been calling it.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:255:
c.append('result->Set("%(argname)s", %(argname)s.ToValue());')
SetWithoutPathExpansion

(need to audit this whole file for that..)

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/generate_h.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_h.py:9: class H_Generator(object):
Same comment applies here as to CCGenerator.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_h.py:17: """Generates the .h code for
self.namespace.
ditto

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_h.py:50: c.append('namespace extensions {')
ditto

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_h.py:100: c.append('// Returns a new
DictionaryValue representing the serialized form')
line too long

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:84: members actually exist.
not sure what this sentence means...

not at google - send to devlin

Another few comments; done for the day. Note that there are quite a few places ...

8 years, 11 months ago (2012-01-13 06:45:53 UTC) #7

calamity

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py File tools/json_schema_compiler/code.py (right): http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/code.py#newcode43 tools/json_schema_compiler/code.py:43: """Concatenate another Code object onto this one. On 2012/01/13 ...

8 years, 11 months ago (2012-01-16 04:01:06 UTC) #8

not at google - send to devlin

Just replying to your comments, will go over the new code now. http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/generate_cc.py File tools/json_schema_compiler/generate_cc.py ...

8 years, 11 months ago (2012-01-17 01:59:23 UTC) #9

Just replying to your comments, will go over the new code now.

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/g...
File tools/json_schema_compiler/generate_cc.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/g...
tools/json_schema_compiler/generate_cc.py:47: # TODO(calamity): Events
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/12 06:01:05, kalman wrote:
> > TODO should be before the end of the namespaces
> 
> ??? So move this TODO up?

Well you'd generate the events after the functions, not after the end of the
namespaces.

It's a nitpick :)

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/2001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:7: class ModelT(object):
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/12 06:01:05, kalman wrote:
> > consider putting this in a model/ directory, with a file for each class.
> 
> Any particular reason for doing this?

Yes, so that you can have
 - a different file for each class (good, easier to navigate code)
 - the model classes in the same place (ditto)

However, it seems just as convenient to have these in the same place,
particularly so that you can have like

from model import Namespace

so I guess it's not considered so important to have each class in its own file.

Yeah, don't worry about it.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:24: class Code(object):
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/13 02:14:09, kalman wrote:
> > is explicitly extending object necessary?
> 
> It defines it as a "new style" class. Not sure if that's necessary though (I
> know you need it for subclassing, but maybe there's something else about it?)

Ah... alright.  If it's like Java, classes automatically/implicitly extend from
object, it's weird to see "class Blah extends Object".  But it seems like python
behaves differently.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:42: def add(self, obj):
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/13 02:14:09, kalman wrote:
> > I wonder if it's more convenient to just merge this into append.  That would
> let
> > functions return either a string or a code block depending on what they were
> > feeling like, and it kind of looks nicer IMO.
> > 
> > def append(self, content=''):
> >   if isinstance(content, str):
> >     self.code.append(((' ' * self.indent_level)) + line).rstrip()
> >   elif isinstance(content, Code):
> >     for line in content.code:
> >       self.append(line)
> >   else:
> >     raise TypeError('blah type not supported blah')
> >   return self
> > 
> > you'd need to carefully document that too, of course.
> 
> Maybe. Might be unpythonic. code.Code is meant to be similar to the list api
> which uses append for adding an element and + for adding another list.

Cool, don't worry about it then.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:100: Raises type error if passed something
that isn't a dict."""
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/13 02:14:09, kalman wrote:
> > "Raises type error" comment is unnecessary.
> 
> The style guide says it should list exceptions it raises? To be honest, these
> should be documented a lot better than they are.

This isn't really an exception though, it's a programmer error.

In fact I think it should be an assertion.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/generate_cc.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:88: c.append('if
(!json_schema_compiler::util::GetOptionalTypes<%(type)s>(*dict,')
On 2012/01/16 04:01:06, calamity wrote:
> On 2012/01/13 02:14:09, kalman wrote:
> > this line is too long.
> > 
> > this makes me think though -- I think it would be nice to have a few global
> > variables that are automatically resolved at the top level (although
probably
> by
> > the Code object, since the Code object needs to know about any global
> > substitutions so that unresolved ones can be error checked, anyway)
> > 
> > Things like
> >  - the path to the utils ('json_schema_compiler::util' in this case)
> >  - the namepace that everything is being generated to ('extensions' in this
> > case)
> > 
> > this line could then become
> > 
> > c.append("if (!%(global.utils)s::GetOptionalTypes(...)")
> > 
> > likewise where you've written "extensions", could be %(global.namespace)s or
> > whatever you've been calling it.
> 
> Yeah, but as always it's a tradeoff between usability and readability.
Wouldn't
> that introduce global state?

Not really, it's not global state because it's not state.  They're constants.  I
think it would be nice to have an explicit global constant system.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/generate_cc.py:104: c.append('  return false;')
On 2012/01/16 04:01:06, calamity wrote:
> Clearly this if block is going to grow. On a dev branch I've pulled this into
> it's own class that deals with interactions with util.cc.

I'm missing the context for this comment.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:116: STRING, FUNDAMENTAL, ARRAY, REF,
CHOICES, OBJECT = range(6)
On 2012/01/16 04:01:06, calamity wrote:
> PropertyType.STRING is unused. On dev branch, FUNDAMENTAL is replaced with
> INTEGER, STRING, DOUBLE, BOOLEAN because checking the json_type is dodgy
(Should
> work as much as possible with code rather than strings).

I'm missing the context of this comment.

not at google - send to devlin

Looking really good, this will be my last main around of comments, I think. http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/model.py ...

8 years, 11 months ago (2012-01-17 05:42:32 UTC) #10

calamity

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/code.py File tools/json_schema_compiler/code.py (right): http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/code.py#newcode100 tools/json_schema_compiler/code.py:100: Raises type error if passed something that isn't a ...

8 years, 11 months ago (2012-01-18 05:43:08 UTC) #11

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:100: Raises type error if passed something
that isn't a dict."""
On 2012/01/17 01:59:24, kalman wrote:
> On 2012/01/16 04:01:06, calamity wrote:
> > On 2012/01/13 02:14:09, kalman wrote:
> > > "Raises type error" comment is unnecessary.
> > 
> > The style guide says it should list exceptions it raises? To be honest,
these
> > should be documented a lot better than they are.
> 
> This isn't really an exception though, it's a programmer error.
> 
> In fact I think it should be an assertion.

Done.

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/10004/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:116: STRING, FUNDAMENTAL, ARRAY, REF,
CHOICES, OBJECT = range(6)
On 2012/01/17 01:59:24, kalman wrote:
> On 2012/01/16 04:01:06, calamity wrote:
> > PropertyType.STRING is unused. On dev branch, FUNDAMENTAL is replaced with
> > INTEGER, STRING, DOUBLE, BOOLEAN because checking the json_type is dodgy
> (Should
> > work as much as possible with code rather than strings).
> 
> I'm missing the context of this comment.

Just a note in case you were wondering why PropertyType.STRING is there.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:38: for tipe in
self.__namespace.types.values():
On 2012/01/17 05:42:32, kalman wrote:
> do you actually need "tipe"?  From what I can tell, Python handles "type" just
> fine.

I guess.. but that's confusing? type is a built in function and assigning type
won't go out of scope until end of function. If somebody wants to use type()
(for debugging for instance) they'll have to figure out what the hell happened.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:163: if prop.item_type.json_type ==
'string':
On 2012/01/17 05:42:32, kalman wrote:
> e.g. when you change the way types are represented, this line would be 
> 
> if prop.item_type.type == PropertyType.STRING
> 
> etc

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:167: % (var, prop.name))
On 2012/01/17 05:42:32, kalman wrote:
> this line can be on the same as the one above

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:268: arg =  'const %(type)s&
%(name)s'
On 2012/01/17 05:42:32, kalman wrote:
> extra space

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
File tools/json_schema_compiler/code_test.py (right):

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/code_test.py:6: import code
On 2012/01/17 05:42:32, kalman wrote:
> from code import Code
> 
> I think for a test it's fine to do that :)

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/code_test.py:8: class TestCode(unittest.TestCase):
On 2012/01/17 05:42:32, kalman wrote:
> class CodeTest

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/01/17 05:42:32, kalman wrote:
> #!/usr/bin/env python

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:4: 
On 2012/01/17 05:42:32, kalman wrote:
> We should probably add a general of comment of what this tool is for and how
to
> use it beyond the 'usage' stuff. Mention its history as a tool for generating
> extension API code.
> 
> See like tools/coverity/coverity.py for inspiration.

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:14: description='Generate C++ boilerplate
code for extension apis from json',
On 2012/01/17 05:42:32, kalman wrote:
> "Generates a C++ model of a JSON schema" or something.  Take out reference to
> extension APIs.

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:15: usage='usage: %prog [options] target
referenced_jsons')
On 2012/01/17 05:42:32, kalman wrote:
> I think a more unixy way of saying something like this is
> 
> usage='usage: %prog [option]... schema [referenced_schema]...'
> 

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:28: sys.exit('Error: No input json file')
On 2012/01/17 05:42:32, kalman wrote:
> Print usage instead of just "Error.." ?

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:38: # Load type dependencies into the
model
On 2012/01/17 05:42:32, kalman wrote:
> fullstop

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:57: os.path.relpath(target_json,
opts.root), filename_suffix)
On 2012/01/17 05:42:32, kalman wrote:
> pull this comment and the os.path.relpath(...) thing into a local variable.
> also "include path" is confusing to me, implies #include or something, it's
just
> a relative path to the source file right?

Include path is the #include path? The relative path from src is being used as
the #include path to the file.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:57: os.path.relpath(target_json,
opts.root), filename_suffix)
On 2012/01/17 05:42:32, kalman wrote:
> pull this comment and the os.path.relpath(...) thing into a local variable.
> also "include path" is confusing to me, implies #include or something, it's
just
> a relative path to the source file right?

Well, it's the output path here, but the #include path for the referenced
jsons...

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:47: if includes.render():
On 2012/01/17 05:42:32, kalman wrote:
> seems weird to render() this here then not use the result.  how about adding a
> method like is_empty to Code?
Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:93: )
On 2012/01/17 05:42:32, kalman wrote:
> blank line here perhaps?

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:115: (c.eblock()
On 2012/01/17 05:42:32, kalman wrote:
> blank line here perhaps?

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:14: def add_namespace(self, json,
root_namespace, parent_path, filename_suffix):
On 2012/01/17 05:42:32, kalman wrote:
> 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.

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.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:15: """Add a namespace's json to the model.
On 2012/01/17 05:42:32, kalman wrote:
> .. "if it has a "compile" property set to true.
> Returns the new namespace, or None if a namespace wasn't added.
> """

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:19: namespace =  Namespace(json,
root_namespace,
On 2012/01/17 05:42:32, kalman wrote:
> extra space.

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:35: self.parent_model = parent_model
On 2012/01/17 05:42:32, kalman wrote:
> what's parent_model used for?
Removed.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:66: self.param = Property(param['name'],
param)
On 2012/01/17 05:42:32, kalman wrote:
> 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)

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:79: self.callback = Callback(param)
On 2012/01/17 05:42:32, kalman wrote:
> assert not self.callback

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:81:
self.params.append(Property(param['name'], param))
On 2012/01/17 05:42:32, kalman wrote:
> just put in an else rather than a continue?

Done.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:83: raise KeyError
On 2012/01/17 05:42:32, kalman wrote:
> 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).

Agreed. Left this as an AssertionError for now.

http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:99: self.json_type = json['$ref']
On 2012/01/17 05:42:32, kalman wrote:
> 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.)

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

not at google - send to devlin

LGTM after you fix up those few things. http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/cc_generator.py File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9114036/diff/21001/tools/json_schema_compiler/cc_generator.py#newcode38 tools/json_schema_compiler/cc_generator.py:38: for ...

8 years, 11 months ago (2012-01-18 06:57:28 UTC) #12

calamity

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cpp_type_generator.py File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cpp_type_generator.py#newcode43 tools/json_schema_compiler/cpp_type_generator.py:43: cpp_type = simple_c_types[prop.type] On 2012/01/18 06:57:28, kalman wrote: > ...

8 years, 11 months ago (2012-01-18 07:30:54 UTC) #13

not at google - send to devlin

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cpp_type_generator.py File tools/json_schema_compiler/cpp_type_generator.py (right): http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cpp_type_generator.py#newcode43 tools/json_schema_compiler/cpp_type_generator.py:43: cpp_type = simple_c_types[prop.type] On 2012/01/18 07:30:54, calamity wrote: > ...

8 years, 11 months ago (2012-01-18 07:49:36 UTC) #14

calamity

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cc_generator.py File tools/json_schema_compiler/cc_generator.py (right): http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/cc_generator.py#newcode127 tools/json_schema_compiler/cc_generator.py:127: c.append('if(!dict->%s)' % cpp_util.get_fundamental_value(prop, On 2012/01/18 06:57:28, kalman wrote: > ...

8 years, 11 months ago (2012-01-18 09:35:18 UTC) #15

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:127: c.append('if(!dict->%s)' %
cpp_util.get_fundamental_value(prop,
On 2012/01/18 06:57:28, kalman wrote:
> space between "if" and "("

Done.

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:162: if
PropertyType.is_fundamental(prop):
On 2012/01/18 06:57:28, kalman wrote:
> perhaps it would be more idiomatic to have is_fundamental be a method on
> PropertyType
> 
> if prop.type.is_fundamental():
>   ...

Done.

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/code.py:27: return bool(self.__code)
On 2012/01/18 06:57:28, kalman wrote:
> newline after this

Done.

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:43: cpp_type =
simple_c_types[prop.type]
On 2012/01/18 07:49:37, kalman wrote:
> On 2012/01/18 07:30:54, calamity wrote:
> > On 2012/01/18 06:57:28, kalman wrote:
> > > Just be explicit, and don't have the simple_c_types thing.  It's less
> magical.
> > > 
> > > if prop.type == PropertyType.BOOLEAN
> > >   cpp_type == 'bool'
> > > elif prop.type == PropertyType.INTEGER
> > >   cpp_type == 'int'
> > > ...
> > 
> > Are you adverse to the python-style switch case?
> > 
> > cpp_type = {
> >       PropertyType.BOOLEAN: 'bool',
> >       PropertyType.INTEGER: 'int',
> >       PropertyType.DOUBLE: 'double',
> >       PropertyType.STRING: 'std::string',
> >     }[prop.type]
> 
> No, though then you'd also need to have PropertyType.ARRAY: 'std::vector<%s>'
to
> be consistent.
> 
> However... I believe that that won't work if you make BOOLEAN/INTEGER/etc
> dictionaries rather than ints.

Apparently you can use your classes as keys of a dict. But I guess I'll stick
with elifs for readability.

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:109: self.type =
fundamental_types[json_type]
On 2012/01/18 06:57:28, kalman wrote:
> ditto: be explicit, don't have fundamental_types

Done.

http://codereview.chromium.org/9114036/diff/21002/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:127: INTEGER, DOUBLE, BOOLEAN, STRING,
ARRAY, REF, CHOICES, OBJECT = range(8)
On 2012/01/18 06:57:28, kalman wrote:
> sorry to keep on messing with you here, but it seems like a safer way to
express
> this would be like
> 
> class PropertyType:
>   class Info:
>     def __init__(self, is_fundamental):
>       self.is_fundamental = is_fundamental
> 
>   INTEGER = Info(True)
>   DOUBLE = Info(True)
>   ...
>   OBJECT = Info(False)
> 
> which would be a Java-style enum.
> 
> all the other code would work the same way, except now you can say
> 
> property.type.is_fundamental rather than PropertyType.is_fundamental(prop)
> 
> and I hate static methods and like instance methods, so that's a nice thing. 
> and that also opens up possibilities for putting other things on the object,
if
> necessary.

Done.

calamity

This is a python tool that generates C++ code from the api json files at ...

8 years, 11 months ago (2012-01-18 11:48:34 UTC) #16

Nico

The gyp files look like a very good start! I'll leave it up to aa ...

8 years, 11 months ago (2012-01-18 17:30:12 UTC) #17

The gyp files look like a very good start!

I'll leave it up to aa if the CL as such makes sense (it adds a lot of code for
the generator, I don't know how much can be deleted once it's in. Also,
metaprogramming makes it easy to generate bloaty code, makes the generated code
hard to debug, etc, so I'm generally not a fan.)

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (right):

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.g...
build/json_schema_compile.gypi:10: 'rules': [
It looks like this rule requires a few variables to be set to be useful. Is
there a way to prevent that?

For example, for --destdir, why can't you use <(INTERMEDIATE_DIR) (or, if you
need these files from multiple targets – which you probably dont
– <(SHARED_INTERMEDIATE_DIR)?

If you can't get rid of all variables, maybe you can add a comment that lists
all variables that need to be set and what they mean? (Kind of like a python
function pydoc string)

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.g...
build/json_schema_compile.gypi:34: '<@(json_schema_files)',
Why do you need to pass in both <(RULE_INPUT_PATH) and <@(json_schema_files)?

As far as I understand, a rule is something that says "For files with this
'extension', run the following command, to produce the files in 'outputs'". The
input file is set to <(RULE_INPUT_PATH), so where you include this both
<(RULE_INPUT_PATH) and <@(json_schema_files) should be the same file (?)

http://codereview.chromium.org/9114036/diff/36003/tools/json_schema_compiler/...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9114036/diff/36003/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:64: api_defs =
json.loads(schema_file.read())
with open(schema, 'r') as schema_file:
  api_defs = json.load(schema_file)

?

calamity

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi File build/json_schema_compile.gypi (right): http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi#newcode10 build/json_schema_compile.gypi:10: 'rules': [ On 2012/01/18 17:30:12, Nico wrote: > It ...

8 years, 11 months ago (2012-01-18 23:46:18 UTC) #18

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (right):

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.g...
build/json_schema_compile.gypi:10: 'rules': [
On 2012/01/18 17:30:12, Nico wrote:
> It looks like this rule requires a few variables to be set to be useful. Is
> there a way to prevent that?
> 
> For example, for --destdir, why can't you use <(INTERMEDIATE_DIR) (or, if you
> need these files from multiple targets – which you probably dont
> –&nbsp;<(SHARED_INTERMEDIATE_DIR)?
> 
> If you can't get rid of all variables, maybe you can add a comment that lists
> all variables that need to be set and what they mean? (Kind of like a python
> function pydoc string)

Yeah, using <(SHARED_INTERMEDIATE_DIR) instead of <(cc_root) works. I wasn't
totally sure what I was doing here so most of it was just taking cues from
build/protoc.gypi.

I could get rid of <(cc_dir) if there's a variable that represents the path to
the directory that the RULE_INPUT file is in. Is there anything like that?

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.g...
build/json_schema_compile.gypi:34: '<@(json_schema_files)',
On 2012/01/18 17:30:12, Nico wrote:
> Why do you need to pass in both <(RULE_INPUT_PATH) and <@(json_schema_files)?
> 
> As far as I understand, a rule is something that says "For files with this
> 'extension', run the following command, to produce the files in 'outputs'".
The
> input file is set to <(RULE_INPUT_PATH), so where you include this both
> <(RULE_INPUT_PATH) and <@(json_schema_files) should be the same file (?)

This was a bit of a hack. Each json could have references to other jsons. So the
<(RULE_INPUT_PATH) is the json we're currently generating which outputs
<(RULE_INPUT_ROOT)_api.cc/h. The <@(json_schema_files) are all the jsons that
comprise the model to ensure dependencies are able to be resolved.

Nico

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi File build/json_schema_compile.gypi (right): http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi#newcode10 build/json_schema_compile.gypi:10: 'rules': [ > I could get rid of <(cc_dir) ...

8 years, 11 months ago (2012-01-18 23:54:42 UTC) #19

calamity

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi File build/json_schema_compile.gypi (right): http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi#newcode10 build/json_schema_compile.gypi:10: 'rules': [ On 2012/01/18 23:54:42, Nico wrote: > > ...

8 years, 11 months ago (2012-01-19 00:29:59 UTC) #20

Nico

> I've looked at the tests for RULE_INPUT_DIRNAME and it really looks like it > ...

8 years, 11 months ago (2012-01-19 00:41:12 UTC) #21

calamity

On 2012/01/19 00:41:12, Nico wrote: > > I've looked at the tests for RULE_INPUT_DIRNAME and ...

8 years, 11 months ago (2012-01-19 00:46:53 UTC) #22

Nico

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi File build/json_schema_compile.gypi (right): http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi#newcode10 build/json_schema_compile.gypi:10: 'rules': [ > I've looked at the tests for ...

8 years, 11 months ago (2012-01-19 00:49:46 UTC) #23

calamity

http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi File build/json_schema_compile.gypi (right): http://codereview.chromium.org/9114036/diff/36003/build/json_schema_compile.gypi#newcode10 build/json_schema_compile.gypi:10: 'rules': [ On 2012/01/19 00:49:46, Nico wrote: > > ...

8 years, 11 months ago (2012-01-19 00:57:07 UTC) #24

Nico

> When I use INTERMEDIATE_DIR, the generated files aren't picked up by the the .cc ...

8 years, 11 months ago (2012-01-19 01:04:23 UTC) #25

Yoyo Zhou

I'm not that familiar with the permissions API; jstritar would best be able to review ...

8 years, 11 months ago (2012-01-19 02:19:40 UTC) #26

Yoyo Zhou

Here are a few more interesting comments. http://codereview.chromium.org/9114036/diff/36003/chrome/browser/extensions/extension_permissions_api.cc File chrome/browser/extensions/extension_permissions_api.cc (right): http://codereview.chromium.org/9114036/diff/36003/chrome/browser/extensions/extension_permissions_api.cc#newcode50 chrome/browser/extensions/extension_permissions_api.cc:50: EXTENSION_FUNCTION_VALIDATE(Contains::Params::Populate(*args_, &params)); ...

8 years, 11 months ago (2012-01-19 23:19:10 UTC) #27

Aaron Boodman

In general: hooray. I'm dragging my feet on the Python review, hoping that Yoyo will ...

8 years, 11 months ago (2012-01-19 23:31:13 UTC) #28

calamity

Did a bit of rework. I just thought I'd clean up the style first and ...

8 years, 11 months ago (2012-01-20 01:10:24 UTC) #29

calamity

Uploaded another patch which hopefully addresses the most of the remaining issues. http://codereview.chromium.org/9114036/diff/36003/chrome/browser/extensions/extension_permissions_api.cc File chrome/browser/extensions/extension_permissions_api.cc ...

8 years, 11 months ago (2012-01-23 05:14:45 UTC) #30

jstritar

nice! http://codereview.chromium.org/9114036/diff/45001/chrome/browser/extensions/extension_permissions_api.cc File chrome/browser/extensions/extension_permissions_api.cc (right): http://codereview.chromium.org/9114036/diff/45001/chrome/browser/extensions/extension_permissions_api.cc#newcode23 chrome/browser/extensions/extension_permissions_api.cc:23: nit: no new line? http://codereview.chromium.org/9114036/diff/45001/chrome/browser/extensions/extension_permissions_api_helpers.cc File chrome/browser/extensions/extension_permissions_api_helpers.cc (right): ...

8 years, 11 months ago (2012-01-23 22:42:07 UTC) #31

Yoyo Zhou

Looking pretty good, a few more comments here and there. http://codereview.chromium.org/9114036/diff/36003/tools/json_schema_compiler/code.py File tools/json_schema_compiler/code.py (right): http://codereview.chromium.org/9114036/diff/36003/tools/json_schema_compiler/code.py#newcode15 ...

8 years, 11 months ago (2012-01-23 23:16:42 UTC) #32

calamity

Updated to HEAD, did a bit of rework and utilized Function::Result where possible. Some function ...

8 years, 11 months ago (2012-01-24 22:57:22 UTC) #33

Yoyo Zhou

LGTM http://codereview.chromium.org/9114036/diff/59001/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc (right): http://codereview.chromium.org/9114036/diff/59001/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc#newcode30 chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:30: scoped_ptr<Permissions> permissions(new Permissions()); I think you can use ...

8 years, 11 months ago (2012-01-26 00:10:38 UTC) #34

calamity

http://codereview.chromium.org/9114036/diff/59001/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc (right): http://codereview.chromium.org/9114036/diff/59001/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc#newcode30 chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:30: scoped_ptr<Permissions> permissions(new Permissions()); On 2012/01/26 00:10:38, Yoyo Zhou wrote: ...

8 years, 11 months ago (2012-01-27 00:46:57 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/9114036/66006

8 years, 11 months ago (2012-01-27 00:49:10 UTC) #36

commit-bot: I haz the power

Presubmit check for 9114036-66006 failed and returned exit status 1. Running presubmit commit checks ...

8 years, 11 months ago (2012-01-27 00:49:23 UTC) #37

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/9114036/67006

8 years, 11 months ago (2012-01-27 00:52:59 UTC) #38

commit-bot: I haz the power

Try job failure for 9114036-67006 (retry) on win_rel for step "compile" (clobber build). It's a ...

8 years, 11 months ago (2012-01-27 01:57:29 UTC) #39

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/9114036/70001

8 years, 11 months ago (2012-01-27 05:05:22 UTC) #40

Change committed as 119405

Issue 9114036: Code generation for extensions api (Closed)

Description

Patch Set 1 #

Patch Set 2 : cosmetic tidying #

Patch Set 3 : added generated files, removed trailing whitespace #

Patch Set 4 : some rework #

Patch Set 5 : more rework #

Patch Set 6 : rework rework #

Patch Set 7 : a fistful of rework #

Patch Set 8 : switch style guides #

Patch Set 9 : switched namespaces, various rework #

Patch Set 10 : updated generated files #

Patch Set 11 : updated branch, utilized Result, small fixes #

Patch Set 12 : silly mistake #

Patch Set 13 : remove generated files examples #

Patch Set 14 : fix windows path issue #

Patch Set 15 : windows path fix #

Messages