Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12221050:
Improvements to JSON Schema Compiler's Dart Generation (Closed)
|
Created:
7 years, 10 months ago by sashab Modified:
7 years, 10 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, benwells Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImprovements to JSON Schema Compiler's Dart Generation
* Allowed overrides for methods.
* Fixed correct detection of base-type choices objects
* Added support for Array types; still no support for inline type definitions.
* Fixed detection of base & serialized types in external namespaces
* Some fixes in generated whitespace
* Fixes to allow type, method & function overrides
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182118
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fixed problem with generated constructors, for both events & types #Patch Set 3 : Various fixes, including adding namespace prefixes to type names. #
Total comments: 26
Patch Set 4 : Style/comment fixes #Messages
Total messages: 11 (0 generated)
Hi, If you could take a look at these changes, that would be great. I carried this version of the generator all the way through to creating a sample app, and it worked (ie it generated code that I was able to use to build an app in Dart). The sooner this is accepted, the better :) Thanks, Sash
some long-winded comments. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:34: self._types = namespace.types I didn't notice that you added this alias... it doesn't really seem very valuable. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:55: (c.Append(LICENSE.strip()) Why strip()? P.S. you could make LICENCE a Code() object and then use Cblock here (automatically adds the blank line) - if you want. I don't mind either way. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:70: if not self._ConcatOverride(c, [type_.name], doc_property=type_): Without looking at the implementation of ConcatOverride: can it return a Code object rather than accepting one to concat onto? Return None if it doesn't? I think I meant to make this comment last time around but forgot. You could be super tricky like override = self._GetOverride([type_.name], doc_property=true) c.Cblock(override if override is not None else self._GenerateType(type_)) but wow ternarys are ugly. Whatever, use your judgement. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:71: c.Concat(self._GenerateType(type_)) I suggest using Cblock() here rather than the c.Append() before. In fixing whitespace things in the C++ compiler I've found that the easiest way to get things right is for Code to generate without any blank lines before/after and let the Concat'ters of the code add any spacing (usually with Cblock if they desire that). Since I presume that you want a blank line between each type definition, use Cblock here. If you always want a blank line between /** Types */ and the first type, do it there (up on line 65). https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:107: self._types[type_.name] = type_ This modifies the original model (since types_ is just a reference into the model). That probably isn't what you want to do. Why is this necessary anyway? https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:173: array_type = self._types[prop.type_.ref_type] I think you should re-use CppTypeGenerator rather than reimplementing the ref logic. And by "reuse" I mean pull out the initialization logic, FindType, and FollowRef (make FindType public) into a new class like TypeGraph. Construct TypeGraph rather than CppTypeGenerator in the compiler, pass it everywhere, and the h/c generators can construct the CppTypeGenerator themselves from an injected TypeGraph. If you read through the c/h generators and look for FollowRef you'll see an idiom where methods right at the start do something like concrete_type = type_helper_.FollowRef(type_) and then deal with the concrete type rather than the ref type. That won't always be the right thing to do obviously - often you'll want to know that it is actually a ref - but when generating the instructions typically you only care about the concrete type. I think many places where you're looking at REF would be moot if you used that idiom. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:574: def _ConcatOverride(self, c, key_chain, doc_property=False): From what I can tell, this isn't always a property. In fact reading the code I assumed it was some flag about whether to document properties. How about like "document_as" or something. Also =False as the default argument seems wrong. Should be None. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:589: for line in contents.strip().split('\n'): why strip()? https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:643: def _IsBaseType(self, type_, list_as_base=False): list_as_base isn't descriptive on its own, I had to come look a this method to see what all the list_as_base things were about. It's also a pretty specialised thing to be passing a generic method like this. I'm not really sure what should be the default (true or false) though true makes a bit more sense to me - so how about a parameter "follow" or "unwrap" (follow=False/unwrap=False) and invert the conditions. Does the same problem exist for other types that wrap other types? Notably ref types (should those be treated the same was as lists?). At times when you don't want to unwrap, do you still want to dig down into choices? I guess my understanding of the code that's being generated isn't deep enough to understand the problem that this solves.
Responded to feedback where possible. Removed a lot of the complex, misunderstood functionality as it isn't actually needed (I was trying to generate files that are not actually available from packaged apps). Also added namespace prefixing to type names, to prevent name clashes between IDL files (since a type's name isn't necessarily unique across all IDL files). Am discussing with the Dart guys the best approach to this, but this will have to do for now. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:55: (c.Append(LICENSE.strip()) On 2013/02/07 01:16:41, kalman wrote: > Why strip()? > > P.S. you could make LICENCE a Code() object and then use Cblock here > (automatically adds the blank line) - if you want. I don't mind either way. The problem is getting the LICENSE (and the triple quotes) to fit inside the code. If you look at where its defined, its hard to fit it to 80 chars since it barely fits within 80 chars itself. Managed to get it to fit, though, removing the need for .strip(). https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:70: if not self._ConcatOverride(c, [type_.name], doc_property=type_): On 2013/02/07 01:16:41, kalman wrote: > Without looking at the implementation of ConcatOverride: can it return a Code > object rather than accepting one to concat onto? Return None if it doesn't? I > think I meant to make this comment last time around but forgot. > > You could be super tricky like > > override = self._GetOverride([type_.name], doc_property=true) > c.Cblock(override if override is not None else self._GenerateType(type_)) > > but wow ternarys are ugly. Whatever, use your judgement. This is pretty much what I wanted to do before. Not super-tricky, but I wanted to get a code object and then be like "if it exists, add it on, blah blah"... But it seemed much simpler to just return T/F. Changed it to return a code object, arguably more readable. Refactored the way getters/setters work to use the nice ternary operators too =) https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:71: c.Concat(self._GenerateType(type_)) On 2013/02/07 01:16:41, kalman wrote: > I suggest using Cblock() here rather than the c.Append() before. In fixing > whitespace things in the C++ compiler I've found that the easiest way to get > things right is for Code to generate without any blank lines before/after and > let the Concat'ters of the code add any spacing (usually with Cblock if they > desire that). > > Since I presume that you want a blank line between each type definition, use > Cblock here. If you always want a blank line between /** Types */ and the first > type, do it there (up on line 65). Don't think I need a blank line between /** Types */ and the first type. The problem with CBlock() is a lot of the places I'm using .Append() need strings, and CBlock() wants a code object. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:107: self._types[type_.name] = type_ On 2013/02/07 01:16:41, kalman wrote: > This modifies the original model (since types_ is just a reference into the > model). That probably isn't what you want to do. > > Why is this necessary anyway? A little complicated... Basically for objects that are actually arrays. In this case, we want a wrapper for the actual object, not the array. May not be needed anymore so removed. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:173: array_type = self._types[prop.type_.ref_type] On 2013/02/07 01:16:41, kalman wrote: > I think you should re-use CppTypeGenerator rather than reimplementing the ref > logic. > > And by "reuse" I mean pull out the initialization logic, FindType, and FollowRef > (make FindType public) into a new class like TypeGraph. Construct TypeGraph > rather than CppTypeGenerator in the compiler, pass it everywhere, and the h/c > generators can construct the CppTypeGenerator themselves from an injected > TypeGraph. > > If you read through the c/h generators and look for FollowRef you'll see an > idiom where methods right at the start do something like > > concrete_type = type_helper_.FollowRef(type_) > > and then deal with the concrete type rather than the ref type. That won't always > be the right thing to do obviously - often you'll want to know that it is > actually a ref - but when generating the instructions typically you only care > about the concrete type. > > I think many places where you're looking at REF would be moot if you used that > idiom. May no longer be needed; temporarily removed. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:574: def _ConcatOverride(self, c, key_chain, doc_property=False): On 2013/02/07 01:16:41, kalman wrote: > From what I can tell, this isn't always a property. In fact reading the code I > assumed it was some flag about whether to document properties. > > How about like "document_as" or something. > > Also =False as the default argument seems wrong. Should be None. Done. https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:589: for line in contents.strip().split('\n'): On 2013/02/07 01:16:41, kalman wrote: > why strip()? Good question. File contents always contain a trailing newline. Since the documentation should be straight before the function definition, all newlines here must be removed (hence removing all the blank lines). Maybe .strip('\n') is better? https://codereview.chromium.org/12221050/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:643: def _IsBaseType(self, type_, list_as_base=False): On 2013/02/07 01:16:41, kalman wrote: > list_as_base isn't descriptive on its own, I had to come look a this method to > see what all the list_as_base things were about. It's also a pretty specialised > thing to be passing a generic method like this. > > I'm not really sure what should be the default (true or false) though true makes > a bit more sense to me - so how about a parameter "follow" or "unwrap" > (follow=False/unwrap=False) and invert the conditions. > > Does the same problem exist for other types that wrap other types? Notably ref > types (should those be treated the same was as lists?). At times when you don't > want to unwrap, do you still want to dig down into choices? > > I guess my understanding of the code that's being generated isn't deep enough to > understand the problem that this solves. Before, lists had to be wrapped to be generated correctly. In this new version, lists don't need to be wrapped, so they are technically a "base type". Choices will remain a base type until we come up with a nice way to handle them using Dart classes (I added a note about that).
lgtm https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:34: self._types = namespace.types it's still weird to me to do this; if you find typing self._namespace.types[foo] too verbose then it'd be less surprising to have a FindType function - particularly if it gets any more complex (e.g. adding types to this, like you've commented below). I think what you have is fine for now though. Just something to keep in mind. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:47: # dictionary. Why? https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:49: def _AddPrefix(self, name): define this below Generate, it seems less important. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:54: # need for this prefixing. sgtm https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:110: # Since enum's are just treated as strings for now, don't generate their enum's -> enums https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:161: else self._GenerateGetterAndSetter(type_, prop)) spacing looks odd to me, either have it 4 from the start of the line or line up under the ( https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:176: else self._GenerateFunction(prop.type_.function)) ditto https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:195: else self._GenerateGetter(type_, prop)) ... like that. cool. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:611: for line in contents.strip().split('\n'): yeah, explicitly strip('\n') would have answered my question, so that sgtm https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:681: for c in type_.choices) 1 line? https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:685: and self._IsBaseType(type_.item_type)) +1 indent https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:713: # TODO: Think of a nice way to generate code for Choices objects in Dart. TODO(sashab) https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:723: if type(type_.parent) is not Function: isinstance(type_.parent, Function) ?
Thanks kalman; dcommitting now. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:34: self._types = namespace.types On 2013/02/12 01:26:55, kalman wrote: > it's still weird to me to do this; if you find typing self._namespace.types[foo] > too verbose then it'd be less surprising to have a FindType function - > particularly if it gets any more complex (e.g. adding types to this, like you've > commented below). > > I think what you have is fine for now though. Just something to keep in mind. Added an explanation. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:47: # dictionary. On 2013/02/12 01:26:55, kalman wrote: > Why? Added explanation. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:49: def _AddPrefix(self, name): On 2013/02/12 01:26:55, kalman wrote: > define this below Generate, it seems less important. Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:54: # need for this prefixing. On 2013/02/12 01:26:55, kalman wrote: > sgtm Discussing with dart team atm. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:110: # Since enum's are just treated as strings for now, don't generate their On 2013/02/12 01:26:55, kalman wrote: > enum's -> enums Nice find =) Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:161: else self._GenerateGetterAndSetter(type_, prop)) On 2013/02/12 01:26:55, kalman wrote: > spacing looks odd to me, either have it 4 from the start of the line or line up > under the ( Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:176: else self._GenerateFunction(prop.type_.function)) On 2013/02/12 01:26:55, kalman wrote: > ditto Sorry, these were by accident. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:195: else self._GenerateGetter(type_, prop)) On 2013/02/12 01:26:55, kalman wrote: > ... like that. cool. Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:611: for line in contents.strip().split('\n'): On 2013/02/12 01:26:55, kalman wrote: > yeah, explicitly strip('\n') would have answered my question, so that sgtm Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:681: for c in type_.choices) On 2013/02/12 01:26:55, kalman wrote: > 1 line? Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:685: and self._IsBaseType(type_.item_type)) On 2013/02/12 01:26:55, kalman wrote: > +1 indent Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:713: # TODO: Think of a nice way to generate code for Choices objects in Dart. On 2013/02/12 01:26:55, kalman wrote: > TODO(sashab) Done. https://codereview.chromium.org/12221050/diff/6001/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:723: if type(type_.parent) is not Function: On 2013/02/12 01:26:55, kalman wrote: > isinstance(type_.parent, Function) ? Done, hope you don't mind the introspection here...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12221050/7003
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12221050/7003
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12221050/7003
Message was sent while issue was closed.
Change committed as 182118 |
