|
|
DescriptionGenerate externs automatically from json/idl files
Continued from https://codereview.chromium.org/511943003/.
BUG=469920
Committed: https://crrev.com/8c3a458ed264db8970f72c71be0e32a2a071ec30
Cr-Commit-Position: refs/heads/master@{#322016}
Patch Set 1 : Patch from https://codereview.chromium.org/511943003 #Patch Set 2 : Edits #
Total comments: 18
Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : Tyler's #
Messages
Total messages: 19 (6 generated)
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org, tbreisacher@chromium.org
Hey Tyler and Dan - since Vitaly's not working on chrome much now, I figured I'd pick this up so we can start using it, and fixing some of the things we've noticed.
btw, tbreisacher@'s ooo https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/create_externs.sh (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/create_externs.sh:1: #!/bin/bash :_( https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:25: pass ^ can't you just omit the __init__ in this case? https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:35: """Generates a Code object with the .dart for the entire namespace. the .dart? https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:42: .Append()) why are these () needed? https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:44: for type_ in self._namespace.types.values(): maybe type_ -> js_type if we really can't use |type| itself? https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:73: c.Append('var ' + type_.simple_name + ';') nit: var = 'var ' + type_.simple_name if self._IsTypeConstructor(type_): var += ' = function()' var += ';' https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:93: for line in type_.description.split('\n'): nit: split('\n') -> splitlines()
Sorry for the roughness; thought you and Tyler had already looked over the original, and so pretty much just re-uploaded it. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/create_externs.sh (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/create_externs.sh:1: #!/bin/bash On 2015/03/19 19:15:02, Dan Beam wrote: > :_( Actually, why is this file even here? Removing. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:25: pass On 2015/03/19 19:15:02, Dan Beam wrote: > ^ can't you just omit the __init__ in this case? Done. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:35: """Generates a Code object with the .dart for the entire namespace. On 2015/03/19 19:15:02, Dan Beam wrote: > the .dart? Done. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:42: .Append()) On 2015/03/19 19:15:03, Dan Beam wrote: > why are these () needed? Because python doesn't use traditional line breaks, so otherwise we can use .Append on the next line. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:44: for type_ in self._namespace.types.values(): On 2015/03/19 19:15:02, Dan Beam wrote: > maybe type_ -> js_type if we really can't use |type| itself? Done. https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:73: c.Append('var ' + type_.simple_name + ';') On 2015/03/19 19:15:02, Dan Beam wrote: > nit: > > var = 'var ' + type_.simple_name > if self._IsTypeConstructor(type_): var += ' = function()' > var += ';' Or even: c.Append('var %s%s;' % (js_type.simple_name, ' = function()' if self._IsTypeConstructor(js_type) else '')) https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:93: for line in type_.description.split('\n'): On 2015/03/19 19:15:03, Dan Beam wrote: > nit: split('\n') -> splitlines() Done.
lgtm https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:42: .Append()) On 2015/03/19 20:59:57, Devlin wrote: > On 2015/03/19 19:15:03, Dan Beam wrote: > > why are these () needed? > > Because python doesn't use traditional line breaks, so otherwise we can use > .Append on the next line. it seems like this is already an expression so it should work without () (otherwise how would line wrapper work anywhere...?) https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:73: c.Append('var ' + type_.simple_name + ';') On 2015/03/19 20:59:57, Devlin wrote: > On 2015/03/19 19:15:02, Dan Beam wrote: > > nit: > > > > var = 'var ' + type_.simple_name > > if self._IsTypeConstructor(type_): var += ' = function()' > > var += ';' > > Or even: > c.Append('var %s%s;' % > (js_type.simple_name, > ' = function()' if self._IsTypeConstructor(js_type) else '')) eh, up to you but Tyler's is probably the easiest to read (but is 4 lines), with mine being 2nd easiest (and 3 lines) and yours being 3 lines but harder to read (in my opinion)
https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:42: .Append()) On 2015/03/23 21:58:42, Dan Beam wrote: > On 2015/03/19 20:59:57, Devlin wrote: > > On 2015/03/19 19:15:03, Dan Beam wrote: > > > why are these () needed? > > > > Because python doesn't use traditional line breaks, so otherwise we can use > > .Append on the next line. > > it seems like this is already an expression so it should work without () > (otherwise how would line wrapper work anywhere...?) TL;DR: Python line wrapping sucks. This is needed (confirmed offline). https://codereview.chromium.org/1006373003/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:73: c.Append('var ' + type_.simple_name + ';') On 2015/03/23 21:58:42, Dan Beam wrote: > On 2015/03/19 20:59:57, Devlin wrote: > > On 2015/03/19 19:15:02, Dan Beam wrote: > > > nit: > > > > > > var = 'var ' + type_.simple_name > > > if self._IsTypeConstructor(type_): var += ' = function()' > > > var += ';' > > > > Or even: > > c.Append('var %s%s;' % > > (js_type.simple_name, > > ' = function()' if self._IsTypeConstructor(js_type) else > '')) > > eh, up to you but Tyler's is probably the easiest to read (but is 4 lines), with > mine being 2nd easiest (and 3 lines) and yours being 3 lines but harder to read > (in my opinion) Well, I think yours will actually be 4, since I don't think one-line if statements are allowed. And the constructing a single string with format is actually quite a bit faster than concatenation. Of course, I can't fool myself into thinking this is anything close to performance-sensitive code, so readability triumphs. I'll take yours over Tyler's, because I tend to think that the fewer if-else branches we have, the better.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1006373003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006373003/60001
still lgtm https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:104: if not properties: return Code() fwiw, 1-line if is already used in this file and i think it's fine
I haven't looked at this in too much detail, this is just a couple things I noticed. Feel free to submit without waiting for an LG from me though. https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:70: var += ';' It looks like 'var' is unused; is there supposed to be a c.Append(var) or something like that? https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:114: # remove last trailing comma TODO(someone): Don't need to remove the comma anymore, if/when https://github.com/google/closure-compiler/issues/796 gets fixed.
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:70: var += ';' On 2015/03/23 23:33:52, Tyler Breisacher (Chromium) wrote: > It looks like 'var' is unused; is there supposed to be a c.Append(var) or > something like that? Swear I had that in there... thanks for the catch. Done. https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:104: if not properties: return Code() On 2015/03/23 23:22:56, Dan Beam wrote: > fwiw, 1-line if is already used in this file and i think it's fine Done.
https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1006373003/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:114: # remove last trailing comma On 2015/03/23 23:33:52, Tyler Breisacher (Chromium) wrote: > TODO(someone): Don't need to remove the comma anymore, if/when > https://github.com/google/closure-compiler/issues/796 gets fixed. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1006373003/#ps80001 (title: "Tyler's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006373003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8c3a458ed264db8970f72c71be0e32a2a071ec30 Cr-Commit-Position: refs/heads/master@{#322016} |