Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12041098:
Initial commit of the Dart Chrome Extension APIs generators (Closed)
|
Created:
7 years, 11 months ago by sashab Modified:
7 years, 10 months ago CC:
chromium-reviews, pam+watch_chromium.org, benwells, Emily Fortuna, sra1, sethladd, blois Base URL:
http://git.chromium.org/chromium/src.git@file_path_bugfix Visibility:
Public. |
DescriptionInitial commit of the Dart Chrome Extension APIs generators
Modified json_schema_compiler to allow a -l option to specify the language.
Also allowed for a -H option to specify a hooks file, which is currently only
supported for Dart.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180845
Patch Set 1 #
Total comments: 21
Patch Set 2 : Fixed tracking branch to be master #Patch Set 3 : Added dart subfolder, with custom hooks for the Chrome.* APIs #Patch Set 4 : Fixed minor style issues; added support for [nodart] IDL flag #
Total comments: 27
Patch Set 5 : Updated master to latest #Patch Set 6 : Fixed to work with json_schema_compiler changes, fixed style & structure issues, and implemented a … #
Total comments: 104
Patch Set 7 : #Patch Set 8 : Cleaned up dart_generator, and added wrappers for callbacks #
Total comments: 43
Patch Set 9 : Refactored json_schema_compiler, moving lang-specific code to its own generators. #Patch Set 10 : Small style & structure fixes to dart_generator, and similar. #
Total comments: 38
Patch Set 11 : Added wrappers for more types of functions (including events, callbacks and lists) #
Total comments: 9
Patch Set 12 : Changed constructor for CppTypeGenerator and updated tests #Patch Set 13 : Fixed .gyp files to reflect new filenames #
Total comments: 13
Patch Set 14 : Fixing cpp_type_generator; still some broken unit tests #Patch Set 15 : blah #Patch Set 16 : Reupload of previous patch #Patch Set 17 : kalman fixes #Patch Set 18 : Kalman fixes 2 (nocompile ignored in bundle mode) #Messages
Total messages: 26 (0 generated)
These are the Dart Chrome Extension generators that will be pulled to Dart for
generating the Chrome.* API wrappers.
Please add more reviewers as necessary; I'm not sure on who should review this
change.
You can run it using:
python compiler.py -l dart ../../chrome/common/extensions/api/app_runtime.idl
To output to a file (well, a directory):
python compiler.py -l dart ../../chrome/common/extensions/api/app_runtime.idl
-d ./out
You can also pass in a custom hooks file:
python compiler.py -l dart ../../chrome/common/extensions/api/app_runtime.idl
-H custom_hooks.dart
Custom hooks files are of the format:
/**
* This file contains custom hooks for the generated Chrome.* APIs.
*
* The format for a hook is:
*
* // START (full type name) (property/method name)
* (code)
* // END
*
* For getters/setters, add 'get' or 'set' after the property name.
*
* The given code will be substituted instead of the generated one.
*
*/
// START app.window.AppWindow contentWindow
// TODO(sashab, sra): Detect whether this is the current window, or an
// external one, and return an appropriately-typed object
WindowBase get contentWindow =>
JS("Window", "#.contentWindow", this._jsObject);
// END
(I think these should live in Dart, but correct me if I'm wrong).
Sasha
Sorry; resending due to wrong reviewer addresses.
https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/co... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/co... tools/json_schema_compiler/compiler.py:95: if lang.lower() == 'c++': I don't think this is needed now, but it would be nice to have factorize out this if and the one below into a separate class - lang_generator maybe. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/co... tools/json_schema_compiler/compiler.py:120: print '%s.h' % out_file You can factorize out the if dest_dir ... else: .... part and use it three times (cc, h and dart). https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/co... tools/json_schema_compiler/compiler.py:227: parser.add_option("-H", "--hooks-file", dest="hooks_file", We should talk about custom hooks offline, I'm not sure why they are being passed in. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:9: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file // lose the (c), Dart -> Chromium. Put at top of file like the other .py files in this folder.
Hi Sasha, What does the generator do so far? What does it support / what will it be used for once it's integrated into Chrome? I'll pre-empt a possible answer: is it for developers who want to write dart programs, but want some kind of type safety when calling the chrome APIs with it? In which case, I think the best way to expose this feature is via the docs server - on each API page have a link to "download dart wrapper" or something. Anyway - I'll review this tomorrow (EOD here), but is there somebody to review the dart code itself (Ben?)? I'll brush up on dart syntax but I'm lost beyond that. In the meantime you might want to sync, since a few days ago I submitted a pretty significant change to the compiler.
Hey, At this point, I'm only vaguely familiar with this code and I'll only be able to provide limited feedback. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:25: def _parseCustomHooksFile(self, filename): Style in other files seems to be _CamelCase. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:143: if not self._IsFunction(t))] Extra ) here. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:156: .Concat(self._GenerateDocumentation(prop)) Align periods. Same for rest of file. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:222: return (c.Eblock("}") The h_generator and cc_generator seem to always use return c for clarity. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:235: Uses triple-quotes for the string.""" Triple quotes to next line? https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:260: `callTarget` is the name of the object to call the function on. The default Use | instead of `. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:270: "%(target)s%(params)s)" Maybe use ("text" "nextline" "lastline") instead? https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:287: "params": params I think convention is to use single quotes where convenient.
Apologies before for not giving a better introduction as to what this change is for. Basically, I've been working recently with the Dart team to write a new piece for their standard library, "dart:chrome" (their std libs are prefixed with dart:). This library will be shipped with the Dart Editor, as well as the Dart SDK, and become available to users in their Dart applications who want to write Chrome v2 packaged apps in Dart, instead of Javascript. This library will be generated from the IDL files we have for the Chrome APIs, and will wrap them (providing a 1:1 mapping, and allowing them to be called natively from within Dart applications). All Javascript types will be converted to native Dart types, and everything will be functionally equivalent to the Javascript versions. This compiler, json_schema_compiler, generates the code needed for that library. I am putting this code in Chrome so that I can re-use the already-build IDL and JSON parsers for reading in the API definitions, but it is NOT used as part of the Chromium build process. Instead, all of this code will be pulled into Dart's repository (via DEPS) and built locally in there, where it will be combined with a few more files and turned into the final library distributable. Later, perhaps, when the Dart VM becomes a part of Chrome, this file may be a part of the Chromium build process. For now, the code lives in here, but it is not used to build Chrome in any way (although the C++-generation is still used, the Dart generation is not). Furthermore, the documentation for these APIs (also taken from the IDL) is automatically added to the generated Dart files, and will be picked up by the Dart SDK documentation generators, and hence appear on Dart's website (under the documentation for the dart:chrome library). I'll also request to provide links to the general Chrome Apps documentation from the Dart page, so users can navigate there if they have general Apps questions. Thank you, and I hope this clarifies the situation a little. I'm working on a design doc at the moment too, which should also make this all a lot clearer. Sasha P.S. Since there may be functions that are not wrappable right now due to serialization/compatibility issues, I have added a 'nodart' flag that you can attach to the front of any API function or property, and it will be excluded from dart generation. It works identically to the 'nocompile' flag, which is used to exclude functions from c++ generation. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:9: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2013/01/25 03:15:32, benwells wrote: > // lose the (c), Dart -> Chromium. Put at top of file like the other .py files > in this folder. The dart files need this header. This license goes at the top of the generated .dart files. Maybe they need 2 headers? https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:25: def _parseCustomHooksFile(self, filename): On 2013/01/25 04:16:01, calamity wrote: > Style in other files seems to be _CamelCase. Done. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:143: if not self._IsFunction(t))] On 2013/01/25 04:16:01, calamity wrote: > Extra ) here. Done, I'm surprised this worked at all :S Thanks https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:156: .Concat(self._GenerateDocumentation(prop)) On 2013/01/25 04:16:01, calamity wrote: > Align periods. Same for rest of file. Done. Sorry, was meant to be like this. =) https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:222: return (c.Eblock("}") On 2013/01/25 04:16:01, calamity wrote: > The h_generator and cc_generator seem to always use > > return c > > for clarity. Done, but there's a return (Code()...) thing below. Is that ok? https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:235: Uses triple-quotes for the string.""" On 2013/01/25 04:16:01, calamity wrote: > Triple quotes to next line? Good spotting, thanks https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:260: `callTarget` is the name of the object to call the function on. The default On 2013/01/25 04:16:01, calamity wrote: > Use | instead of `. Done. https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:270: "%(target)s%(params)s)" On 2013/01/25 04:16:01, calamity wrote: > Maybe use ("text" > "nextline" > "lastline") > instead? Didn't know you could do that. Thanks! :) https://codereview.chromium.org/12041098/diff/1/tools/json_schema_compiler/da... tools/json_schema_compiler/dart_generator.py:287: "params": params On 2013/01/25 04:16:01, calamity wrote: > I think convention is to use single quotes where convenient. Done this where possible :)
I didn't look at dart_generator.py too closely - I think it will end up changing a bit when you sync - the changes I made to the compiler tightens up the scope of the model, for example, previously many things were modelled as a Property when really they should have been a Type (e.g. choices), and that was pretty pervasive. I'd also like to understand the custom hooks things a bit better. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/code.py:66: def SblockBare(self): The nice thing about the other operations (with the exception of Comment) Append Cblock Eblock Concat is that they're the same length. When chaining together calls to Code, they line up vertically, which makes it much easier to scan the code generation. I would suggest that instead of adding these methods you change Sblock and Eblock to take None as their default "line" argument, then have if line is not None: self.Append(line) self._indent_level += self._indent_size return self https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:95: if lang.lower() == 'c++': Unfortunately, now compiler.py has three personalities - there is a branch up in the stack on bundle vs not-bundle mode, and a branch lower in the stack about c++ vs dart mode. However, really there are just 3 generators. A Generator takes a namespace (plus dependencies) and returns Code. Well actually, really there are 4 generators, it just happens that c++ implies both h_generator and cc_generator. Anyhow, I think this all can be factored much more cleanly, and now is as good a time as any to do that. Let's formalise what a Generator is in code: it has a Generate method which takes a namespace and returns a Code object. I'd also go and do a bit of renaming so that this makes more sense, like - SchemaBundleGenerator -> CppBundleGenerator, rather than have GenerateAPIHeaders etc methods have an api_h_generator property - Add a CppBundleGenerator with similar property h_generator and cc_generator This file can then look something like if opts.bundle: cpp_bundle_generator = CppBundleGenerator(...) generators = [ ('generated_api.h': cpp_bundle_generator.api_h_generator), ('generated_schemas.cc': cpp_bundle_generator.schemas_cc_generator), ('generated_schemas.h': cpp_bundle_generator.schemas_h_generator) ] elif opts.lang == 'c++': cpp_generator = CppGenerator(...) generators = [ ('%s.h' % namespace.name), cpp_generator.h_generator), ('%s.cc' % namespace.name), cpp_generator.cc_generator) ] elif opts.lang == 'dart': generators = [ ('%s.dart' % namespace.name), DartGenerator(...) ] for filename, generator in generators: with open(os.path.join(dest_dir, filename)) as f: f.write(generator.Generate(namespace).Render()) With that in mind I also think it makes more sense to have just a single --mode flag on the compiler, with cpp_bundle, cpp, and dart options. P.S. now that the Generate methods take a namespace rather than the constructor, I'd suggest you make those generators look something like class HGenerator(object): def __init__(self, type_generator): self._type_generator = type_generator def Generate(self, namespace); return _Generator(self._type_generator, namespace).Generate() class _Generator(object): # exactly the contents that HGenerator used to have https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:188: else: I think the time has come to delete this branch https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/dart/custom_hooks.dart (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart/custom_hooks.dart:15: Whatever happens to these files - could we put each customisation in its own file? I can see this file getting pretty enormous, and the // START // END thing could get awkward. However, as the JS customisation stuff works, it would be better to solve this problem at the language/library level rather than at the compiler level. Compiler complexity is worse than code complexity. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:15: import copy unused? https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:18: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file use datetime.now().year rather than 2013 https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:130: .Append(' */') is generating these comments useful? https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:253: return c could you add this functionality to Code in a general way (it already has a Comment method which is supposed to do this kind of thing) https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:397: def _GeneratePropertySignature(self, prop, prependThis = False, style is prepend_this=False i.e. underscore style not camelcase, no spaces either side of = https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:546: return prop.type_ == PropertyType.REF I think that the value of these methods isn't that high - and besides a lot of this stuff will be unnecessary-ish when you sync https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:578: def _GetPropertyType(self, prop): when you sync, _GetDartType(self, type_) https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/json_schema.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/json_schema.py:18: def DeleteNocompileNodes(item, lang='c++'): how about renaming this to DeleteNodes(item, key) and let callers specify either nodart or nocompile.
Fixed a lot of the style & structure issues, as well as accounted for the recent changes to json_schema_compiler. Began implementing the new override system, discussed in: https://docs.google.com/a/google.com/document/d/184brgpJXSfndyHacBXp-gQl-9fyB... with example override files in the Dart repository: https://codereview.chromium.org/12088036/ These files will help wrap functions whose wrappers cannot be automatically generated (for whatever reason). The choice to move away from the [nodart] specifier, and towards a parameter-based model is mainly to isolate the customisations to a single repository: if [nodart] is used, both Chromium and Dart would need to be updated. This way, only the custom overrides in Dart have to be updated, and the IDL files in Chromium don't need to be modified at all. Feedback & suggestions welcome. I may also further refactor json_schema_compiler at a later time. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/code.py:66: def SblockBare(self): On 2013/01/25 18:14:33, kalman wrote: > The nice thing about the other operations (with the exception of Comment) > > Append > Cblock > Eblock > Concat > > is that they're the same length. When chaining together calls to Code, they line > up vertically, which makes it much easier to scan the code generation. > > I would suggest that instead of adding these methods you change Sblock and > Eblock to take None as their default "line" argument, then have > > if line is not None: > self.Append(line) > self._indent_level += self._indent_size > return self Sure - I'm not going to change the default line argument to None, though - Code.Append() is already being used elsewhere, and it means to add a blank line. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/dart/custom_hooks.dart (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart/custom_hooks.dart:15: On 2013/01/25 18:14:33, kalman wrote: > Whatever happens to these files - could we put each customisation in its own > file? I can see this file getting pretty enormous, and the // START // END thing > could get awkward. > > However, as the JS customisation stuff works, it would be better to solve this > problem at the language/library level rather than at the compiler level. > Compiler complexity is worse than code complexity. Done by fixing the way these files are passed in. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:15: import copy On 2013/01/25 18:14:33, kalman wrote: > unused? Fixed; nice spotting https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:18: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2013/01/25 18:14:33, kalman wrote: > use datetime.now().year rather than 2013 Good idea! :) https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:130: .Append(' */') On 2013/01/25 18:14:33, kalman wrote: > is generating these comments useful? I think it helps make the final code more readable. especially when there are lots of methods/properties. I'll compare with dart:html for reference. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:253: return c On 2013/01/25 18:14:33, kalman wrote: > could you add this functionality to Code in a general way (it already has a > Comment method which is supposed to do this kind of thing) Saw the Comment method before I read this; a much better idea (it even wraps long comments!!). Changed :) https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:397: def _GeneratePropertySignature(self, prop, prependThis = False, On 2013/01/25 18:14:33, kalman wrote: > style is prepend_this=False > > i.e. underscore style not camelcase, no spaces either side of = Done, and fixed spacing issue for ='s everywhere in this file. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:546: return prop.type_ == PropertyType.REF On 2013/01/25 18:14:33, kalman wrote: > I think that the value of these methods isn't that high - and besides a lot of > this stuff will be unnecessary-ish when you sync Got rid of this one, but kept the other two (since their meaning is not completely clear just from the if-statement). https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/dart_generator.py:578: def _GetPropertyType(self, prop): On 2013/01/25 18:14:33, kalman wrote: > when you sync, _GetDartType(self, type_) You mean I should rename this function, and make it take a type_ instead? https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/json_schema.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/json_schema.py:18: def DeleteNocompileNodes(item, lang='c++'): On 2013/01/25 18:14:33, kalman wrote: > how about renaming this to DeleteNodes(item, key) and let callers specify either > nodart or nocompile. Done; moved this language-specific decision to compiler.py.
Like I said, I'm really nervous adding that much code to compiler.py, please consider something like what I wrote up last round of reviews. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/code.py:66: def SblockBare(self): On 2013/01/29 08:27:13, sasha_b wrote: > On 2013/01/25 18:14:33, kalman wrote: > > The nice thing about the other operations (with the exception of Comment) > > > > Append > > Cblock > > Eblock > > Concat > > > > is that they're the same length. When chaining together calls to Code, they > line > > up vertically, which makes it much easier to scan the code generation. > > > > I would suggest that instead of adding these methods you change Sblock and > > Eblock to take None as their default "line" argument, then have > > > > if line is not None: > > self.Append(line) > > self._indent_level += self._indent_size > > return self > > Sure - I'm not going to change the default line argument to None, though - > Code.Append() is already being used elsewhere, and it means to add a blank line. Well, if Append didn't add a line given a blank argument then it'd be a no-op. I wasn't suggesting to change Append, just Sblock and Eblock. And yeah, I think that would be neater than explicitly writing None. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:38: # custom_dart_directory option. sgtm https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:10: from collections import defaultdict unused https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:22: datetime.now().year) indent this line? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:28: def __init__(self, namespace, custom_dart_folder): IMO like you said in the doc, "overrides" would be a nice name. also, please use "dir" not "folder". https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:34: if custom_dart_folder: If custom_dart_folder is optional then make the argument custom_dart_folder=None to indicate it. Also, use "if custom_dart_folder is not None" since "if custom_dart_folder" will also fail on empty-string, and that's not something that we need to handle. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:43: x= self._namespace delete 3 lines above here? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:53: # Add all types. comment not useful https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:63: # Add all events. ditto https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:73: # Add main class for this file. ditto https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:100: add_public_constructor = True add_public_constructor = any(self._IsFunction(p) for p in type_.properties.values()) https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:106: constructor_fields = [] constructor_fields = [self._GeneratePropertySignature(p) for p in type_.properties.values()] https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:111: omitBasicTypes = False)) prepend_this = False -> prepend_this=False omitBasicTypes = False -> omit_basic_types=False however, since they're False by default, you can leave it out. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:113: # Add the public constructor. I think you can leave this comment out, the variable is called add_public_constructor and there's a dart comment anyway. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:127: # Add the private constructor. ditto https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:136: if not self._IsFunction(t)] nit: p would be a slighty more appropriate temporary variable than t https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:148: overwrite_key = '%s.%s' % (type_.name, prop.name) overwrite -> override? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:149: if overwrite_key in self._custom_dart: A common idiom is like: override = self._custom_dart.get('%s.%s' % (type_.name, prop.name)) if override is not None: ... https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:151: if contents.strip(): Why is this guard necessary? If it is for some reason possible for empty strings to end up in custom_dart, that check should be done while populating the map. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:184: # TODO(sashab): What to do in this situation? Unserializable type. throw an exception? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:195: c.Append(line) This pattern (reading from custom dart and then appending it) is repeated 3 times, make it a function. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:208: # Now add all the function properties. s/function properties/methods/ ? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:238: if not hasattr(prop, 'description'): "hasattr" should no longer be necessary (and we should avoid reflection if possible). props should always have at least a None description, so check "is not None" https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:241: if prop.description: oh, there's a check here anyway. I prefer the check above https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:257: def _GenerateProxyCall(self, function, callTarget='this._jsObject'): call_target https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:281: params = ', ' + ', '.join(params_wrapped) the adding comma stuff is a bit icky, could you just add all the comma-separated things above into a list and then ', '.join them at once, rather than separating out the components? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:382: 'namespace_name': self._namespace.unix_name substitution only used once, just %s it above? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:391: withGetKeyword=False): functionsAsObjects -> functions_as_objects withGetKeyword -> with_get_keyword That said I can't find anywhere where these parameters are used. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:429: return self._GenerateFunctionSignature(prop.type_.function, prepend_this) for optional arguments, be explicit, it's easier to navigate code (and since optional arguments aren't ordered, it's easier to understand what's going on). So specify prepend_this=prepend_this, silly as it looks. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:433: name_parts[0] = 'this.' + name_parts[0] might be a bit more concise as name = '%s%s' % ('this.' if prepend_this else '', prop.simple_name) much as Python's ternary ifs confuse me a little bit each time I see one. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:435: name_parts = ['get'] + name_parts somewhat likewise, like if with_get_keyword: name = 'get %s' % name https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:470: # Get function name. comment here and above seems unnecessary https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:473: function_name = 'this.' + function_name use 'this.%s' % function_name rather than string concatenation https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:479: getattr(function, 'callback', None)) getattr/reflection shouldn't be necessary, if there's no callback then it should be None, so just function.callback. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:513: # join params comment doesn't look right, also, can this all be just return ', '.join(params_req + params_opt) ? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:532: return name.rsplit('.', 1)[1] I think there are utils for this nand GetNamespace already in schema_util.py. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:543: prop.type_.property_type == PropertyType.ANY) return prop.type_.property_name in [PropertyType.OBJECT, PropertyType.ANY] https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:552: return False return self._GetPropertyType(prop) in ['bool', 'num', ...] https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:555: """Given the name of a referenced type, returns the type object for that unused? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:567: if prop == None: is None https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:573: if type_ == None: for all comparisons here, use "is" not == https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:583: dart_type = 'bool' return 'bool' https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:585: dart_type = 'int' return 'int' (etc) https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:595: # TODO(sashab): What is Choices? Is it closer to a Map? who are you asking? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? who are you asking? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:605: if hasattr(prop, 'item_type'): if it's an ARRAY it will always have an item_type, don't need hasattr. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:615: return dart_type.strip() why do you need to .strip()? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/json_schema.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/json_schema.py:50: not a relevant change
Cleaned up dart_generator quite a bit; now working on refactoring json_schema_compiler (mainly compiler.py) from the ground up. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:95: if lang.lower() == 'c++': Unfortunately, it doesn't seem that simple, for a few reasons: * The current cpp generation seems to pull in all dependencies for a file, as well as the file itself, whereas the bundle generator doesn't check for dependencies - This also means that the cpp generator can output multiple schemas (multiple cpp files) for one IDL file - There's a comment about whether this is necessary for IDL. For simplicity, I've taken it out for now. * Depending on the language, nodes may or may not be deleted (like nocompile). For now, this is hardcoded as an if-statement checking for c++ options (bundle or lang=c++). https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:38: # custom_dart_directory option. On 2013/01/29 16:37:08, kalman wrote: > sgtm Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:10: from collections import defaultdict On 2013/01/29 16:37:08, kalman wrote: > unused Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:22: datetime.now().year) On 2013/01/29 16:37:08, kalman wrote: > indent this line? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:28: def __init__(self, namespace, custom_dart_folder): On 2013/01/29 16:37:08, kalman wrote: > IMO like you said in the doc, "overrides" would be a nice name. > > also, please use "dir" not "folder". Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:34: if custom_dart_folder: On 2013/01/29 16:37:08, kalman wrote: > If custom_dart_folder is optional then make the argument custom_dart_folder=None > to indicate it. Also, use "if custom_dart_folder is not None" since "if > custom_dart_folder" will also fail on empty-string, and that's not something > that we need to handle. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:43: x= self._namespace On 2013/01/29 16:37:08, kalman wrote: > delete 3 lines above here? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:53: # Add all types. On 2013/01/29 16:37:08, kalman wrote: > comment not useful Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:63: # Add all events. On 2013/01/29 16:37:08, kalman wrote: > ditto Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:73: # Add main class for this file. On 2013/01/29 16:37:08, kalman wrote: > ditto Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:100: add_public_constructor = True On 2013/01/29 16:37:08, kalman wrote: > add_public_constructor = any(self._IsFunction(p) > for p in type_.properties.values()) Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:106: constructor_fields = [] On 2013/01/29 16:37:08, kalman wrote: > constructor_fields = [self._GeneratePropertySignature(p) > for p in type_.properties.values()] Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:111: omitBasicTypes = False)) On 2013/01/29 16:37:08, kalman wrote: > prepend_this = False -> prepend_this=False > omitBasicTypes = False -> omit_basic_types=False > > however, since they're False by default, you can leave it out. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:113: # Add the public constructor. On 2013/01/29 16:37:08, kalman wrote: > I think you can leave this comment out, the variable is called > add_public_constructor and there's a dart comment anyway. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:127: # Add the private constructor. On 2013/01/29 16:37:08, kalman wrote: > ditto Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:136: if not self._IsFunction(t)] On 2013/01/29 16:37:08, kalman wrote: > nit: p would be a slighty more appropriate temporary variable than t Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:148: overwrite_key = '%s.%s' % (type_.name, prop.name) On 2013/01/29 16:37:08, kalman wrote: > overwrite -> override? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:149: if overwrite_key in self._custom_dart: On 2013/01/29 16:37:08, kalman wrote: > A common idiom is like: > > override = self._custom_dart.get('%s.%s' % (type_.name, prop.name)) > if override is not None: > ... > Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:151: if contents.strip(): On 2013/01/29 16:37:08, kalman wrote: > Why is this guard necessary? If it is for some reason possible for empty > strings to end up in custom_dart, that check should be done while populating the > map. If you want to "hide" a function completely, you can make an empty file with the name of the function. In this case, we don't want to print the function's documentation. I'm also not sure the best place to put this; moving it to when the tree is generated wouldn't really help, as you could be overwriting the getter for the class, but not the setter, in which case you still want the documentation (and you can't just remove it from the tree, since you need the information for the setter). The code logic for this actually isn't quite right - it should really be: if override is not blank: write docs do override else: if override_get is not blank: write docs do override for get if override_set is not blank: if override_get is blank: write docs do override for set Can you think of a better way to do this? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:184: # TODO(sashab): What to do in this situation? Unserializable type. On 2013/01/29 16:37:08, kalman wrote: > throw an exception? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:195: c.Append(line) On 2013/01/29 16:37:08, kalman wrote: > This pattern (reading from custom dart and then appending it) is repeated 3 > times, make it a function. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:208: # Now add all the function properties. On 2013/01/29 16:37:08, kalman wrote: > s/function properties/methods/ ? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:238: if not hasattr(prop, 'description'): On 2013/01/29 16:37:08, kalman wrote: > "hasattr" should no longer be necessary (and we should avoid reflection if > possible). props should always have at least a None description, so check "is > not None" Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:241: if prop.description: On 2013/01/29 16:37:08, kalman wrote: > oh, there's a check here anyway. I prefer the check above Hopefully I've made it a little clearer now. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:257: def _GenerateProxyCall(self, function, callTarget='this._jsObject'): On 2013/01/29 16:37:08, kalman wrote: > call_target Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:281: params = ', ' + ', '.join(params_wrapped) On 2013/01/29 16:37:08, kalman wrote: > the adding comma stuff is a bit icky, could you just add all the comma-separated > things above into a list and then ', '.join them at once, rather than separating > out the components? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:382: 'namespace_name': self._namespace.unix_name On 2013/01/29 16:37:08, kalman wrote: > substitution only used once, just %s it above? Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:391: withGetKeyword=False): On 2013/01/29 16:37:08, kalman wrote: > functionsAsObjects -> functions_as_objects > withGetKeyword -> with_get_keyword > > That said I can't find anywhere where these parameters are used. Fixed the camelcase problems. Removed the unused parameters (they were needed in a previous version). https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:429: return self._GenerateFunctionSignature(prop.type_.function, prepend_this) On 2013/01/29 16:37:08, kalman wrote: > for optional arguments, be explicit, it's easier to navigate code (and since > optional arguments aren't ordered, it's easier to understand what's going on). > So specify prepend_this=prepend_this, silly as it looks. Removed this parameter pass anyway, but note taken. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:433: name_parts[0] = 'this.' + name_parts[0] On 2013/01/29 16:37:08, kalman wrote: > might be a bit more concise as > > name = '%s%s' % ('this.' if prepend_this else '', prop.simple_name) > > much as Python's ternary ifs confuse me a little bit each time I see one. Not used anymore anyway. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:435: name_parts = ['get'] + name_parts On 2013/01/29 16:37:08, kalman wrote: > somewhat likewise, like > > if with_get_keyword: > name = 'get %s' % name Not used anymore anyway. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:470: # Get function name. On 2013/01/29 16:37:08, kalman wrote: > comment here and above seems unnecessary Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:473: function_name = 'this.' + function_name On 2013/01/29 16:37:08, kalman wrote: > use 'this.%s' % function_name rather than string concatenation No longer used, but note taken. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:479: getattr(function, 'callback', None)) On 2013/01/29 16:37:08, kalman wrote: > getattr/reflection shouldn't be necessary, if there's no callback then it should > be None, so just function.callback. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:513: # join params On 2013/01/29 16:37:08, kalman wrote: > comment doesn't look right, also, can this all be just > > return ', '.join(params_req + params_opt) ? Fixed comment. Optional parameters have to be in square brackets. This logic is quite messy. no params: '' 2 required params: 'x, y' 2 optional params: '[a, b]' 2 req & 2 opt params: 'x, y, [a, b]' Tried to clean this up a bit. Let me know what you think. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:532: return name.rsplit('.', 1)[1] On 2013/01/29 16:37:08, kalman wrote: > I think there are utils for this nand GetNamespace already in schema_util.py. Removed; replaced usages with the ones from schema_util. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:543: prop.type_.property_type == PropertyType.ANY) On 2013/01/29 16:37:08, kalman wrote: > return prop.type_.property_name in [PropertyType.OBJECT, PropertyType.ANY] Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:552: return False On 2013/01/29 16:37:08, kalman wrote: > return self._GetPropertyType(prop) in ['bool', 'num', ...] Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:555: """Given the name of a referenced type, returns the type object for that On 2013/01/29 16:37:08, kalman wrote: > unused? Removed. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:567: if prop == None: On 2013/01/29 16:37:08, kalman wrote: > is None Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:573: if type_ == None: On 2013/01/29 16:37:08, kalman wrote: > for all comparisons here, use "is" not == Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:583: dart_type = 'bool' On 2013/01/29 16:37:08, kalman wrote: > return 'bool' Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:585: dart_type = 'int' On 2013/01/29 16:37:08, kalman wrote: > return 'int' > > (etc) Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:595: # TODO(sashab): What is Choices? Is it closer to a Map? On 2013/01/29 16:37:08, kalman wrote: > who are you asking? Anyone who knows? ^^ What is the Dart equivalent of a Choices property type? Is it closer to a Map, or an object? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? On 2013/01/29 16:37:08, kalman wrote: > who are you asking? That was directed at myself; not sure what to do here - in the case of a generic object, it may not be serializable to JS from Dart. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:605: if hasattr(prop, 'item_type'): On 2013/01/29 16:37:08, kalman wrote: > if it's an ARRAY it will always have an item_type, don't need hasattr. Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:615: return dart_type.strip() On 2013/01/29 16:37:08, kalman wrote: > why do you need to .strip()? Old fix; removed. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/json_schema.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/json_schema.py:50: On 2013/01/29 16:37:08, kalman wrote: > not a relevant change This was part of the merge from head.
Some more comments, gotta leave work, looking forward to the compiler.py changes. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:95: if lang.lower() == 'c++': On 2013/01/30 05:26:03, sasha_b wrote: > Unfortunately, it doesn't seem that simple, for a few reasons: > * The current cpp generation seems to pull in all dependencies for a file, as > well as the file itself, whereas the bundle generator doesn't check for > dependencies > - This also means that the cpp generator can output multiple schemas > (multiple cpp files) for one IDL file > - There's a comment about whether this is necessary for IDL. For simplicity, > I've taken it out for now. > * Depending on the language, nodes may or may not be deleted (like nocompile). > For now, this is hardcoded as an if-statement checking for c++ options (bundle > or lang=c++). I think these are surmountable. If a generator needs to output multiple files, then just make it multiple generators (I think the code snippet I generated handles this). It didn't handle the nocompile thing - but in that code snippet, we could build the namespace for each generator and make the node removal one of the variables, like generators = [ (('%s.dart' % namespace.name), 'nodart', DartGenerator(...)) ] https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:151: if contents.strip(): On 2013/01/30 05:26:03, sasha_b wrote: > On 2013/01/29 16:37:08, kalman wrote: > > Why is this guard necessary? If it is for some reason possible for empty > > strings to end up in custom_dart, that check should be done while populating > the > > map. > > If you want to "hide" a function completely, you can make an empty file with the > name of the function. In this case, we don't want to print the function's > documentation. > > I'm also not sure the best place to put this; moving it to when the tree is > generated wouldn't really help, as you could be overwriting the getter for the > class, but not the setter, in which case you still want the documentation (and > you can't just remove it from the tree, since you need the information for the > setter). > > The code logic for this actually isn't quite right - it should really be: > > if override is not blank: > write docs > do override > else: > if override_get is not blank: > write docs > do override for get > if override_set is not blank: > if override_get is blank: > write docs > do override for set > > Can you think of a better way to do this? So is this an alternative to specifying nodart? https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:595: # TODO(sashab): What is Choices? Is it closer to a Map? On 2013/01/30 05:26:03, sasha_b wrote: > On 2013/01/29 16:37:08, kalman wrote: > > who are you asking? > > Anyone who knows? ^^ > > What is the Dart equivalent of a Choices property type? Is it closer to a Map, > or an object? Heh. I was just making sure you weren't asking me :) I'd suggest chatting to calamity who knows a lot about these things (he was original author of the compiler) and... somebody who knows dart. Though for some background: JS isn't typesafe so you can just pass any old value in obviously. C++ is typesafe so we generate a struct like (.. if there was a field "foo" with choice between int and bool): struct FooType { int as_int; bool as_bool; } So... yep, those are at least a couple of options. Another would be to try to generate a different version of the function for each possible parameter list, but I think this would get really complicated and not work in many cases (in fact, I recently rewrote a bunch of the schema compiler to not do this since it prevented us from supporting a few things). I suspect your best bet is to think of a syntactically sugary way of supporting the struct variant, which my knowledge of dart is insufficient to help with. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? On 2013/01/30 05:26:03, sasha_b wrote: > On 2013/01/29 16:37:08, kalman wrote: > > who are you asking? > > That was directed at myself; not sure what to do here - in the case of a generic > object, it may not be serializable to JS from Dart. uninformed idea: could you just not generate type information here, since dart does optional typing? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:32: self._custom_dart = {} _type_overrides? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:135: if not self._TryAppendOverride(c, type_, prop, '.get', add_doc=True): key_suffix='.get' https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:156: if not self._TryAppendOverride(c, type_, prop, '.set'): key_suffix='.set' https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:170: if self._IsFunction(t)] might fit on 1 line? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:236: .Eblock(None) like I said earlier, just make None the default? The only thing this would affect is 2 places in h_generator.py, and you could update those to add an extra Append(). https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:428: allow_optional=True): if it can't fit on 1 line then align parameters vertically https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:439: # Params lists (required & optional), to be joined with ,'s. commas? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:440: # FIXME(sashab): assuming all optional params come after required ones I can't parse this sentence. Should it be like "Don't assume all optional parameters come after required ones"? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:448: params_req.append(p_sig) this isn't ignoring optional parameters, it's making them required? Either code or comment needs to change? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:461: params_opt[-1] = '%s]' % params_opt[-1] a bit bizarre but much better, and I can't think of a non-bizarre way to do it. Please add to comment what you put in the code review comment though. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:463: ', '.join(params_opt)] single line https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:465: return ', '.join(p for p in param_sets if p) why the "if p"? I.e. why can't it be ', '.join(param_sets)? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:467: def _TryAppendOverride(self, c, type_, prop, key_suffix='', add_doc=False): TryAppendOverride is a bit of an awkward name. Perhaps ConcatOverride or something. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:470: If there is, adds a newline, appends the override code, and returns True. "adds a newline" -> doesn't seem like this should be the job of this method. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:485: maybe early-exit here too, if it's empty: if contents.strip() == '': # Passing in an empty file here is the idiom for disabling # compilation of a function for dart. return Code() if prop is not None: c.Concat(self._GenreateDocumentation(prop)) ... https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:487: if prop is not None: although TBH I find this idiom a bit strange. People will see these files checked in empty and will be like "what", or they might say "/// Left empty to disable blah" -- and I think the comment thing is what we might want to support. So something to think about. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:492: c.Append() seem to be always appending here, so pull out above the "if prop is not None" - however, it seems like the extra newline is something that callers should handle https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:514: def _GetPropertyType(self, prop, container_type=False): call this _GetDartType and pass in the type rather than the property. Callers can decide whether to pass in prop.type or prop.type.item_type (no need for container_type). https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:552: return 'Object' I wrote a long thing in response to what you said, but it occurs to me that you could also go the JS-esque route (if that's possible) and just make this untyped. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:561: if type_.item_type: there should always be a list type, this check should be unnecessary? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... File tools/json_schema_compiler/schema_util.py (right): https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/schema_util.py:16: return SplitNamespace(ref)[-1] I'm confused, what does "base namespace" mean? Isn't this the same as StripNamespace since SplitNamespace returns a 2-tuple?
Refactored json_schema_compiler from the ground-up, and fixed some small structure & style issues with dart_generator. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:151: if contents.strip(): On 2013/01/31 02:08:48, kalman wrote: > On 2013/01/30 05:26:03, sasha_b wrote: > > On 2013/01/29 16:37:08, kalman wrote: > > > Why is this guard necessary? If it is for some reason possible for empty > > > strings to end up in custom_dart, that check should be done while populating > > the > > > map. > > > > If you want to "hide" a function completely, you can make an empty file with > the > > name of the function. In this case, we don't want to print the function's > > documentation. > > > > I'm also not sure the best place to put this; moving it to when the tree is > > generated wouldn't really help, as you could be overwriting the getter for the > > class, but not the setter, in which case you still want the documentation (and > > you can't just remove it from the tree, since you need the information for the > > setter). > > > > The code logic for this actually isn't quite right - it should really be: > > > > if override is not blank: > > write docs > > do override > > else: > > if override_get is not blank: > > write docs > > do override for get > > if override_set is not blank: > > if override_get is blank: > > write docs > > do override for set > > > > Can you think of a better way to do this? > > So is this an alternative to specifying nodart? Exactly. The good thing about this is its all in the one repo. That is, if we want to add "nodart" to a function, we just have to worry about maintaining that in Dart's repo, not in Chromium's. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:595: # TODO(sashab): What is Choices? Is it closer to a Map? On 2013/01/31 02:08:48, kalman wrote: > On 2013/01/30 05:26:03, sasha_b wrote: > > On 2013/01/29 16:37:08, kalman wrote: > > > who are you asking? > > > > Anyone who knows? ^^ > > > > What is the Dart equivalent of a Choices property type? Is it closer to a Map, > > or an object? > > Heh. I was just making sure you weren't asking me :) I'd suggest chatting to > calamity who knows a lot about these things (he was original author of the > compiler) and... somebody who knows dart. > > Though for some background: JS isn't typesafe so you can just pass any old value > in obviously. C++ is typesafe so we generate a struct like (.. if there was a > field "foo" with choice between int and bool): > > struct FooType { > int as_int; > bool as_bool; > } > > So... yep, those are at least a couple of options. Another would be to try to > generate a different version of the function for each possible parameter list, > but I think this would get really complicated and not work in many cases (in > fact, I recently rewrote a bunch of the schema compiler to not do this since it > prevented us from supporting a few things). > > I suspect your best bet is to think of a syntactically sugary way of supporting > the struct variant, which my knowledge of dart is insufficient to help with. Object will support all types, but obviously stricter is better. By "struct", it sounds like you really mean "union". I'll investigate a Dart equivalent of this, otherwise I think the best option is to just use "Map", and hope they have the right keys/values etc. Maybe another option is to create a new class that specialises Map, and allows setting of just one of these variables. I'll ask vsm. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? On 2013/01/31 02:08:48, kalman wrote: > On 2013/01/30 05:26:03, sasha_b wrote: > > On 2013/01/29 16:37:08, kalman wrote: > > > who are you asking? > > > > That was directed at myself; not sure what to do here - in the case of a > generic > > object, it may not be serializable to JS from Dart. > > uninformed idea: could you just not generate type information here, since dart > does optional typing? Yes, that's why its "Object" if it can't figure out what type to use (all objects in dart extend Object). The problem is security, and stability - consider this generated code: function Foo(Object bar) { serialize bar; do something in js with bar; return stuff; } Now, if the user calls this function like this: Foo(1); It'll be fine, since 1 is serializable. But, if they call it like this: Foo(new MyObj()); It will fail, since MyObj is not serializable (Dart has little introspection and cannot serialize arbitrary objects). So, in the case where the IDL doesn't have a type definition, it may be worth either adding one or writing a custom override for it, just to get type safety. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:32: self._custom_dart = {} On 2013/01/31 02:08:48, kalman wrote: > _type_overrides? Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:135: if not self._TryAppendOverride(c, type_, prop, '.get', add_doc=True): On 2013/01/31 02:08:48, kalman wrote: > key_suffix='.get' Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:156: if not self._TryAppendOverride(c, type_, prop, '.set'): On 2013/01/31 02:08:48, kalman wrote: > key_suffix='.set' Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:170: if self._IsFunction(t)] On 2013/01/31 02:08:48, kalman wrote: > might fit on 1 line? Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:236: .Eblock(None) On 2013/01/31 02:08:48, kalman wrote: > like I said earlier, just make None the default? The only thing this would > affect is 2 places in h_generator.py, and you could update those to add an extra > Append(). Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:428: allow_optional=True): On 2013/01/31 02:08:48, kalman wrote: > if it can't fit on 1 line then align parameters vertically Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:439: # Params lists (required & optional), to be joined with ,'s. On 2013/01/31 02:08:48, kalman wrote: > commas? Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:440: # FIXME(sashab): assuming all optional params come after required ones On 2013/01/31 02:08:48, kalman wrote: > I can't parse this sentence. Should it be like "Don't assume all optional > parameters come after required ones"? Haha, yes, you're right. Fixed https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:448: params_req.append(p_sig) On 2013/01/31 02:08:48, kalman wrote: > this isn't ignoring optional parameters, it's making them required? Either code > or comment needs to change? Sorry, comment changed. And I also changed the name of the argument to "convert_optional", and reversed its usage (False-->True). https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:461: params_opt[-1] = '%s]' % params_opt[-1] On 2013/01/31 02:08:48, kalman wrote: > a bit bizarre but much better, and I can't think of a non-bizarre way to do it. > Please add to comment what you put in the code review comment though. Done. Or should this rather be in the function signature's comment? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:463: ', '.join(params_opt)] On 2013/01/31 02:08:48, kalman wrote: > single line Done. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:465: return ', '.join(p for p in param_sets if p) On 2013/01/31 02:08:48, kalman wrote: > why the "if p"? I.e. why can't it be ', '.join(param_sets)? This is part of the bizarre logic. If there are no optional parameters, this prevents a trailing comma, ie '(x, y,)'. Similarly, if there are no required parameters, this prevents a leading comma, ie '(, [a, b])'. Put this in the comment too. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:467: def _TryAppendOverride(self, c, type_, prop, key_suffix='', add_doc=False): On 2013/01/31 02:08:48, kalman wrote: > TryAppendOverride is a bit of an awkward name. Perhaps ConcatOverride or > something. It returns True if it appended the override, False if not, hence the "Try". Renamed it to ConcatOverride for now. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:470: If there is, adds a newline, appends the override code, and returns True. On 2013/01/31 02:08:48, kalman wrote: > "adds a newline" -> doesn't seem like this should be the job of this method. Ok, the reason this is needed is in case the override is blank. If the override is blank ('nodart' equivalent), we don't want to add a newline at all. But we don't know whether the override is blank until we reach this method. If I move the logic for adding the newline to _outside_ this method, it will add newlines for 'nodart'ed methods, too. I originally had a "_HasOverride()" method, and a lot of this logic was in the calling function. But they you had to call "_ApplyOverride()" with the same arguments, which meant re-passing the key and stuff, and it didn't feel right. Can you think of a better way I could structure it? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:485: On 2013/01/31 02:08:48, kalman wrote: > maybe early-exit here too, if it's empty: > > if contents.strip() == '': > # Passing in an empty file here is the idiom for disabling > # compilation of a function for dart. > return Code() > > if prop is not None: > c.Concat(self._GenreateDocumentation(prop)) > ... Good idea! :D https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:487: if prop is not None: On 2013/01/31 02:08:48, kalman wrote: > although TBH I find this idiom a bit strange. People will see these files > checked in empty and will be like "what", or they might say "/// Left empty to > disable blah" -- and I think the comment thing is what we might want to support. > So something to think about. It's a little tough, because its unclear: does putting a comment in one of the files mean you _want_ it copied into the final library, or not? The libraries are public-facing, so its important. benwells did suggest an @nodoc flag which you can put at the top of the file, which overrides the default and doesn't show the documentation for a property. Maybe after the flag you could put more stuff you don't want to appear, e.g.: app.window.AppWindow.dart: @nodoc Disabled because an AppWindow is secret. app.window.Intent.dart: @nodoc The docs online are old, use the ones below instead. /// blah blah function() { blah } So blank files would now keep the documentation, and you would need "@nodoc" to actually remove them. Now a file with "@nodoc" is closer to the equivalent of [nodart]. What do you think? https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:492: c.Append() On 2013/01/31 02:08:48, kalman wrote: > seem to be always appending here, so pull out above the "if prop is not None" - > however, it seems like the extra newline is something that callers should handle Thanks, fixed this one; see comment above about callers. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:514: def _GetPropertyType(self, prop, container_type=False): On 2013/01/31 02:08:48, kalman wrote: > call this _GetDartType and pass in the type rather than the property. Callers > can decide whether to pass in prop.type or prop.type.item_type (no need for > container_type). Yup, was going to do this before. Made a wrapper _GetPropertyType in case prop is null. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:552: return 'Object' On 2013/01/31 02:08:48, kalman wrote: > I wrote a long thing in response to what you said, but it occurs to me that you > could also go the JS-esque route (if that's possible) and just make this > untyped. 'Object' is the equivalent of untyped. See other comment. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:561: if type_.item_type: On 2013/01/31 02:08:48, kalman wrote: > there should always be a list type, this check should be unnecessary? After investigating, you're right - its invalid IDL when there's no type defined. Fixed. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... File tools/json_schema_compiler/schema_util.py (right): https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/schema_util.py:16: return SplitNamespace(ref)[-1] On 2013/01/31 02:08:48, kalman wrote: > I'm confused, what does "base namespace" mean? Isn't this the same as > StripNamespace since SplitNamespace returns a 2-tuple? The case I was thinking of was: "app.window.AppWindow." Base namespace = 'AppWindow', not 'window' Looked at the implementation of SplitNamespace, and you're right - it's .rsplit, and it has a limit of 1 split. Removed this.
Looking really nice, just need to update the blank-override thing based on the email discussion, then we're done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? On 2013/01/31 04:41:40, sasha_b wrote: > On 2013/01/31 02:08:48, kalman wrote: > > On 2013/01/30 05:26:03, sasha_b wrote: > > > On 2013/01/29 16:37:08, kalman wrote: > > > > who are you asking? > > > > > > That was directed at myself; not sure what to do here - in the case of a > > generic > > > object, it may not be serializable to JS from Dart. > > > > uninformed idea: could you just not generate type information here, since dart > > does optional typing? > Yes, that's why its "Object" if it can't figure out what type to use (all > objects in dart extend Object). > > The problem is security, and stability - consider this generated code: > > function Foo(Object bar) { > serialize bar; > do something in js with bar; > return stuff; > } > > Now, if the user calls this function like this: > > Foo(1); > > It'll be fine, since 1 is serializable. > > But, if they call it like this: > > Foo(new MyObj()); > > It will fail, since MyObj is not serializable (Dart has little introspection and > cannot serialize arbitrary objects). > > So, in the case where the IDL doesn't have a type definition, it may be worth > either adding one or writing a custom override for it, just to get type safety. Yep, I get that - but I presume ultimately that this will be passed to the JS bindings (right?) which does runtime type checking. Runtime type check is obviously not ideal but it's only in rare situations and it's certainly no worse than what devs already need to deal with. ^^^ that all said, is there a misunderstanding happening here? Objects are objects in the JS sense not the dart sense, and should generate structs. It's a very similar problem to the Choices one, except here I really do mean structs not union (though above I actually *did* mean struct since I presumed dart didn't support union). So something like this, borrowing from what little dart I know: say there is an object (JS object!) with properties "chocolate" which is a bool and "blueberries" which is an int. In JS you'd write this like // Not enough chocolate or blueberries. chrome.muffins.bake({ chocolate: false, blueberries: 20 }); and the runtime type checking would make sure that when you call bake() chocolate is a bool and blueberries is an int. In C++ there is no equivalent of a JS object, so it would generate struct BakeType { bool chocolate; int blueberries; }; (and if you were actually able to make these calls from C++ it would look something like...) BakeType args; args.chocolate = false; args.blueberries = 20; chrome::muffins::bake(args); So what I was suggesting here is either do the JS thing and let the runttime type stuff figure it out (seems like a reasonable way to start) or be clever and do the C++ thing. From what I just read about dart 2 minutes ago, this might look like class BakeType { bool chocolate int blueberries } chrome.muffins.bake(new BakeType() ..chocolate = false ..blueberries = 20 ) ... Now choices remains a problem, and if dart doesn't support that explicitly then I'd just be tempted to make it object and let JS runtime type checking kick in. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:601: dart_type = prop.type_.instance_of or 'Object' ^^^ That said you could also generate for choices (if bake took an argument "temperature" that was either a bool for hot/cold or a number): class TemperatureType { static TemperatureType bool_choice(bool value) { // No idea if this is valid. return new TemperatureType()..as_bool = value } static TemperatureType int_choice(int value) { return new TemperatureType()..as_int = value } bool as_bool int as_int } chrome.muffins.bake(new BakeType() ..chocolate = true ..blueberries = 100 ..temperature = TemperatureType.int_choice(200) ) since I presume you couldn't have class TemperatureType { TemperatureType(bool value) { as_bool = value } TemperatureType(int value) { as_int = value } bool as_bool int as_int } dart seems too dynamic-y to support that... don't quote me on it. https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/21002/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:461: params_opt[-1] = '%s]' % params_opt[-1] On 2013/01/31 04:41:40, sasha_b wrote: > On 2013/01/31 02:08:48, kalman wrote: > > a bit bizarre but much better, and I can't think of a non-bizarre way to do > it. > > Please add to comment what you put in the code review comment though. > > Done. Or should this rather be in the function signature's comment? What you have is good. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:19: from cpp_generator import CppGenerator I like this idiom. Let's make all the imports which import a single class the same. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:63: help='''The language to generate the output in.''') Ok, one last complaint: --bundle and --lang are separate opts, but I don't think it makes sense for them to be. They're not on separate axes: bundle is a C++ specific concept. It's really just a question of how we're generating code. (I actually already commented this, but anyway...) So I'd replace both --bundle and --lang with --generator which is one of cpp, dart, or cpp-bundle. (I find typing c++ awkward, I suppose you could support both, but that would look messy). And you know what's cool? It looks like optparse supports this very use case: http://docs.python.org/2/library/optparse.html#optparse.Option.choices Taking out the --bundle option would mean updating a gyp file somewhere, but it's nbd. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:95: # TODO(miket): do we need this in IDL? I think this comment might be out of date now. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:161: else: I am so happy right now! Factored perfection! https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_bundle_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:123: class GenerateAPIHeader(object): _APIHGenerator would be more consistent with naming throughout the compiler. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:171: class GenerateSchemasHeader(object): _SchemasHGenerator https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:196: class GenerateSchemasCC(object): _SchemasCCGenerator https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_generator.py:6: import h_generator yeah, from cc_generator import CCGenerator etc. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_type_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:45: # TODO(sashab,kalman): Is this the correct behaviour of self._namespace? What was wrong with the way it was before? https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:7: Pass 'dart' with the -l flag to compiler.py to activate the use of this library. maybe that'll change. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:207: extra \n https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:273: params = ["'%s'" % self._GetPropertyType(function.returns), You have inspired me to write https://codereview.chromium.org/12176002/ which changes function.returns to be a type not a property, so you'll probably need to change this (and the other place). https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:275: ', '.join(['#'] * n_params)), can fit on 1 line https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:508: return Code() This is going to change, right? https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:534: def _GetPropertyType(self, prop): Since it's so few places that call this (and about to be even fewer) just make callers do prop.type_ themselves, I don't think the method is worth the extra jump my brain needs to make while reading the code. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:539: return 'void' Once function.returns is a type I don't think this can ever be true.
Added wrappers for many more types of callbacks (including events) and support for more types of parameters (lists of base types, lists of custom types, callbacks). This eliminated the need for some custom wrapper code in app_runtime.idl, and some other places. Also changed the --lang and --bundle parameters to --generator, and modified the gyp build files accordingly. Finally, took into account the new changes from https://codereview.chromium.org/12176002/, and integrated them. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/code.py:66: def SblockBare(self): On 2013/01/29 16:37:08, kalman wrote: > On 2013/01/29 08:27:13, sasha_b wrote: > > On 2013/01/25 18:14:33, kalman wrote: > > > The nice thing about the other operations (with the exception of Comment) > > > > > > Append > > > Cblock > > > Eblock > > > Concat > > > > > > is that they're the same length. When chaining together calls to Code, they > > line > > > up vertically, which makes it much easier to scan the code generation. > > > > > > I would suggest that instead of adding these methods you change Sblock and > > > Eblock to take None as their default "line" argument, then have > > > > > > if line is not None: > > > self.Append(line) > > > self._indent_level += self._indent_size > > > return self > > > > Sure - I'm not going to change the default line argument to None, though - > > Code.Append() is already being used elsewhere, and it means to add a blank > line. > > Well, if Append didn't add a line given a blank argument then it'd be a no-op. I > wasn't suggesting to change Append, just Sblock and Eblock. And yeah, I think > that would be neater than explicitly writing None. Done. https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/3003/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:188: else: On 2013/01/25 18:14:33, kalman wrote: > I think the time has come to delete this branch Done. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:600: # TODO(sashab): Work out a mapped type name? On 2013/02/02 00:45:47, kalman wrote: > On 2013/01/31 04:41:40, sasha_b wrote: > > On 2013/01/31 02:08:48, kalman wrote: > > > On 2013/01/30 05:26:03, sasha_b wrote: > > > > On 2013/01/29 16:37:08, kalman wrote: > > > > > who are you asking? > > > > > > > > That was directed at myself; not sure what to do here - in the case of a > > > generic > > > > object, it may not be serializable to JS from Dart. > > > > > > uninformed idea: could you just not generate type information here, since > dart > > > does optional typing? > > Yes, that's why its "Object" if it can't figure out what type to use (all > > objects in dart extend Object). > > > > The problem is security, and stability - consider this generated code: > > > > function Foo(Object bar) { > > serialize bar; > > do something in js with bar; > > return stuff; > > } > > > > Now, if the user calls this function like this: > > > > Foo(1); > > > > It'll be fine, since 1 is serializable. > > > > But, if they call it like this: > > > > Foo(new MyObj()); > > > > It will fail, since MyObj is not serializable (Dart has little introspection > and > > cannot serialize arbitrary objects). > > > > So, in the case where the IDL doesn't have a type definition, it may be worth > > either adding one or writing a custom override for it, just to get type > safety. > > Yep, I get that - but I presume ultimately that this will be passed to the JS > bindings (right?) which does runtime type checking. Runtime type check is > obviously not ideal but it's only in rare situations and it's certainly no worse > than what devs already need to deal with. > > ^^^ that all said, is there a misunderstanding happening here? Objects are > objects in the JS sense not the dart sense, and should generate structs. It's a > very similar problem to the Choices one, except here I really do mean structs > not union (though above I actually *did* mean struct since I presumed dart > didn't support union). > > So something like this, borrowing from what little dart I know: say there is an > object (JS object!) with properties "chocolate" which is a bool and > "blueberries" which is an int. In JS you'd write this like > > // Not enough chocolate or blueberries. > chrome.muffins.bake({ chocolate: false, blueberries: 20 }); > > and the runtime type checking would make sure that when you call bake() > chocolate is a bool and blueberries is an int. > > In C++ there is no equivalent of a JS object, so it would generate > > struct BakeType { > bool chocolate; > int blueberries; > }; > > (and if you were actually able to make these calls from C++ it would look > something like...) > > BakeType args; > args.chocolate = false; > args.blueberries = 20; > chrome::muffins::bake(args); > > So what I was suggesting here is either do the JS thing and let the runttime > type stuff figure it out (seems like a reasonable way to start) or be clever and > do the C++ thing. From what I just read about dart 2 minutes ago, this might > look like > > class BakeType { > bool chocolate > int blueberries > } > > chrome.muffins.bake(new BakeType() > ..chocolate = false > ..blueberries = 20 > ) > > ... > > Now choices remains a problem, and if dart doesn't support that explicitly then > I'd just be tempted to make it object and let JS runtime type checking kick in. Ohhhh right, I understand what you're saying now. Yup, this is all happening already - every type gets a generated "container" which has all the properties it needs. It also has runtime type-checking like you suggested. The comment (# TODO(sashab): Work out a mapped type name?) is now out-of-date. https://codereview.chromium.org/12041098/diff/17001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:601: dart_type = prop.type_.instance_of or 'Object' On 2013/02/02 00:45:47, kalman wrote: > ^^^ > > That said you could also generate for choices (if bake took an argument > "temperature" that was either a bool for hot/cold or a number): > > class TemperatureType { > static TemperatureType bool_choice(bool value) { > // No idea if this is valid. > return new TemperatureType()..as_bool = value > } > > static TemperatureType int_choice(int value) { > return new TemperatureType()..as_int = value > } > > bool as_bool > int as_int > } > > chrome.muffins.bake(new BakeType() > ..chocolate = true > ..blueberries = 100 > ..temperature = TemperatureType.int_choice(200) > ) > > since I presume you couldn't have > > class TemperatureType { > TemperatureType(bool value) { > as_bool = value > } > TemperatureType(int value) { > as_int = value > } > bool as_bool > int as_int > } > > dart seems too dynamic-y to support that... don't quote me on it. This is a great idea. Let me talk this over with the dart guys. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:19: from cpp_generator import CppGenerator On 2013/02/02 00:45:48, kalman wrote: > I like this idiom. Let's make all the imports which import a single class the > same. Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:63: help='''The language to generate the output in.''') On 2013/02/02 00:45:48, kalman wrote: > Ok, one last complaint: --bundle and --lang are separate opts, but I don't think > it makes sense for them to be. They're not on separate axes: bundle is a C++ > specific concept. It's really just a question of how we're generating code. > > (I actually already commented this, but anyway...) > > So I'd replace both --bundle and --lang with --generator which is one of cpp, > dart, or cpp-bundle. (I find typing c++ awkward, I suppose you could support > both, but that would look messy). > > And you know what's cool? It looks like optparse supports this very use case: > > http://docs.python.org/2/library/optparse.html#optparse.Option.choices > > Taking out the --bundle option would mean updating a gyp file somewhere, but > it's nbd. Done. Not sure what nbd means, but I updated the .gyp files. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:95: # TODO(miket): do we need this in IDL? On 2013/02/02 00:45:48, kalman wrote: > I think this comment might be out of date now. What does it mean? *Is* it needed in IDL? I'm pretty sure the answer is no, but I think it *is* needed in JSON (where you can actually specify a dependencies property). https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:161: else: On 2013/02/02 00:45:48, kalman wrote: > I am so happy right now! Factored perfection! =D https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_bundle_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:123: class GenerateAPIHeader(object): On 2013/02/02 00:45:48, kalman wrote: > _APIHGenerator would be more consistent with naming throughout the compiler. Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:171: class GenerateSchemasHeader(object): On 2013/02/02 00:45:48, kalman wrote: > _SchemasHGenerator Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_bundle_generator.py:196: class GenerateSchemasCC(object): On 2013/02/02 00:45:48, kalman wrote: > _SchemasCCGenerator Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_generator.py:6: import h_generator On 2013/02/02 00:45:48, kalman wrote: > yeah, from cc_generator import CCGenerator etc. Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/cpp_type_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:45: # TODO(sashab,kalman): Is this the correct behaviour of self._namespace? On 2013/02/02 00:45:48, kalman wrote: > What was wrong with the way it was before? The old code did stuff like: (for a single schema) type_generator = cpp_type_generator.CppTypeGenerator( root_namespace, namespace, namespace.unix_name) for referenced_namespace in api_model.namespaces.values(): if referenced_namespace == namespace: continue type_generator.AddNamespace( referenced_namespace, referenced_namespace.unix_name) (for a bundle schema) type_generator = cpp_type_generator.CppTypeGenerator(root_namespace) for referenced_namespace in api_model.namespaces.values(): type_generator.AddNamespace( referenced_namespace, referenced_namespace.unix_name) (see https://codereview.chromium.org/12041098/patch/3003/5006) This just allows you to write this the second way, but also set the ._namespace variable (which is used by the cpp-generator later). I'm guessing the bundle generator doesn't use ._namespace, so it was still working even though it wasn't being set. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:7: Pass 'dart' with the -l flag to compiler.py to activate the use of this library. On 2013/02/02 00:45:48, kalman wrote: > maybe that'll change. Doesn't belong in here anyway. Removed. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:207: On 2013/02/02 00:45:48, kalman wrote: > extra \n Nice spotting :) Removed. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:273: params = ["'%s'" % self._GetPropertyType(function.returns), On 2013/02/02 00:45:48, kalman wrote: > You have inspired me to write https://codereview.chromium.org/12176002/ which > changes function.returns to be a type not a property, so you'll probably need to > change this (and the other place). Hehe, good change :) Done. Not sure how to pull your change into this one, though... https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:275: ', '.join(['#'] * n_params)), On 2013/02/02 00:45:48, kalman wrote: > can fit on 1 line Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:508: return Code() On 2013/02/02 00:45:48, kalman wrote: > This is going to change, right? Yes, I changed it now. And I removed the "adding a blank line" functionality from this function; it now goes in the calling function, where it belongs. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:534: def _GetPropertyType(self, prop): On 2013/02/02 00:45:48, kalman wrote: > Since it's so few places that call this (and about to be even fewer) just make > callers do prop.type_ themselves, I don't think the method is worth the extra > jump my brain needs to make while reading the code. This is here in case prop is None, in which case prop.type_ throws an exception. Looking at your comment below, and all the places where its called, I don't think this can happen. Removed. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:539: return 'void' On 2013/02/02 00:45:48, kalman wrote: > Once function.returns is a type I don't think this can ever be true. Done.
lgtm https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:63: help='''The language to generate the output in.''') On 2013/02/04 05:09:27, sasha_b wrote: > On 2013/02/02 00:45:48, kalman wrote: > > Ok, one last complaint: --bundle and --lang are separate opts, but I don't > think > > it makes sense for them to be. They're not on separate axes: bundle is a C++ > > specific concept. It's really just a question of how we're generating code. > > > > (I actually already commented this, but anyway...) > > > > So I'd replace both --bundle and --lang with --generator which is one of cpp, > > dart, or cpp-bundle. (I find typing c++ awkward, I suppose you could support > > both, but that would look messy). > > > > And you know what's cool? It looks like optparse supports this very use case: > > > > http://docs.python.org/2/library/optparse.html#optparse.Option.choices > > > > Taking out the --bundle option would mean updating a gyp file somewhere, but > > it's nbd. > > Done. Not sure what nbd means, but I updated the .gyp files. nbd = no big deal https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:95: # TODO(miket): do we need this in IDL? On 2013/02/04 05:09:27, sasha_b wrote: > On 2013/02/02 00:45:48, kalman wrote: > > I think this comment might be out of date now. > > What does it mean? *Is* it needed in IDL? > > I'm pretty sure the answer is no, but I think it *is* needed in JSON (where you > can actually specify a dependencies property). I more mean that this code executes independently of whether it's IDL or not, so the comment is moot. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:273: params = ["'%s'" % self._GetPropertyType(function.returns), On 2013/02/04 05:09:27, sasha_b wrote: > On 2013/02/02 00:45:48, kalman wrote: > > You have inspired me to write https://codereview.chromium.org/12176002/ which > > changes function.returns to be a type not a property, so you'll probably need > to > > change this (and the other place). > > Hehe, good change :) Done. > > Not sure how to pull your change into this one, though... Oh, it should have been submitted already.. https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:138: type_generator.AddNamespace( You know what? I think it's odd that this happens here. The type generator could just be constructed with the Model object and do it itself, with an optional root namespace parameter (which could be set if only one filename was specified). That should fix the problems we're having over there (the weird first-namespace-added-becomes-root). https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:274: for l in lists_to_proxy: l is always a bad idea for a variable name https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:300: """Given a function, returns True if this function's needs to be proxied, s/this function's/it/ P.S. please briefly explain what it means to be proxied (and why it's necessary) https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:614: if prop_type is None: this condition is not possible https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:618: return StripNamespace(type_.ref_type) Yeah, cpp_type_generator contains a lot of that logic (as you probably noticed). It would make sense to factor out the language-independent logic from there if you need to start supporting that.
Changed the constructor for CppTypeGenerator and updated its tests to work with this new constructor. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:95: # TODO(miket): do we need this in IDL? On 2013/02/04 17:21:06, kalman wrote: > On 2013/02/04 05:09:27, sasha_b wrote: > > On 2013/02/02 00:45:48, kalman wrote: > > > I think this comment might be out of date now. > > > > What does it mean? *Is* it needed in IDL? > > > > I'm pretty sure the answer is no, but I think it *is* needed in JSON (where > you > > can actually specify a dependencies property). > > I more mean that this code executes independently of whether it's IDL or not, so > the comment is moot. Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:95: # TODO(miket): do we need this in IDL? On 2013/02/04 17:21:06, kalman wrote: > On 2013/02/04 05:09:27, sasha_b wrote: > > On 2013/02/02 00:45:48, kalman wrote: > > > I think this comment might be out of date now. > > > > What does it mean? *Is* it needed in IDL? > > > > I'm pretty sure the answer is no, but I think it *is* needed in JSON (where > you > > can actually specify a dependencies property). > > I more mean that this code executes independently of whether it's IDL or not, so > the comment is moot. Done. https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/17004/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:273: params = ["'%s'" % self._GetPropertyType(function.returns), On 2013/02/04 17:21:06, kalman wrote: > On 2013/02/04 05:09:27, sasha_b wrote: > > On 2013/02/02 00:45:48, kalman wrote: > > > You have inspired me to write https://codereview.chromium.org/12176002/ > which > > > changes function.returns to be a type not a property, so you'll probably > need > > to > > > change this (and the other place). > > > > Hehe, good change :) Done. > > > > Not sure how to pull your change into this one, though... > > Oh, it should have been submitted already.. It doesn't look like its landed? https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:138: type_generator.AddNamespace( On 2013/02/04 17:21:06, kalman wrote: > You know what? I think it's odd that this happens here. The type generator could > just be constructed with the Model object and do it itself, with an optional > root namespace parameter (which could be set if only one filename was > specified). That should fix the problems we're having over there (the weird > first-namespace-added-becomes-root). Done. https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... File tools/json_schema_compiler/dart_generator.py (right): https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:274: for l in lists_to_proxy: On 2013/02/04 17:21:06, kalman wrote: > l is always a bad idea for a variable name Done. https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:300: """Given a function, returns True if this function's needs to be proxied, On 2013/02/04 17:21:06, kalman wrote: > s/this function's/it/ > > P.S. please briefly explain what it means to be proxied (and why it's necessary) Done. https://codereview.chromium.org/12041098/diff/34001/tools/json_schema_compile... tools/json_schema_compiler/dart_generator.py:614: if prop_type is None: On 2013/02/04 17:21:06, kalman wrote: > this condition is not possible Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12041098/50001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12041098/49021
something like this https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:112: os.path.relpath(referenced_schema_path, opts.root)) TBH I think that setting a namespace as default for the first argument is wrong, since the dependencies are loaded first. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:115: for target_namespace, schema_filename in zip(api_defs, filenames): something here like default_namespace = None for target_namespace, schema_filename in zip(api_defs, filename): ... namespace = ... if default_namespace is None: default_namespace = namespace https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:135: type_generator = CppTypeGenerator(api_model, opts.namespace) CppTypeGenerator(api_model, opts.namespace, default_namespace=default_namespace) https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... File tools/json_schema_compiler/cpp_type_generator.py (right): https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:28: def __init__(self, model, root_namespace=''): I don't know if root_namespace needs to be optional. TBH it's also a silly name for what it is, but whatever. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:36: self._namespace = None I meant for this to be like self._namespace = default_namespace https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:43: def AddNamespace(self, namespace, cpp_namespace): I don't think that AddNamespace needs to be called from anywhere, so just inline it in the constructor. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:51: self._namespace = namespace And this logic shouldn't be necessary.
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:115: for target_namespace, schema_filename in zip(api_defs, filenames): On 2013/02/05 00:47:46, kalman wrote: > something here like > > default_namespace = None > for target_namespace, schema_filename in zip(api_defs, filename): > ... > namespace = ... > if default_namespace is None: > default_namespace = namespace > Done. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/compiler.py:135: type_generator = CppTypeGenerator(api_model, opts.namespace) On 2013/02/05 00:47:46, kalman wrote: > CppTypeGenerator(api_model, opts.namespace, default_namespace=default_namespace) Done. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... File tools/json_schema_compiler/cpp_type_generator.py (right): https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:28: def __init__(self, model, root_namespace=''): On 2013/02/05 00:47:46, kalman wrote: > I don't know if root_namespace needs to be optional. > > TBH it's also a silly name for what it is, but whatever. Done. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:36: self._namespace = None On 2013/02/05 00:47:46, kalman wrote: > I meant for this to be like > > self._namespace = default_namespace Done. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:43: def AddNamespace(self, namespace, cpp_namespace): On 2013/02/05 00:47:46, kalman wrote: > I don't think that AddNamespace needs to be called from anywhere, so just inline > it in the constructor. Done. https://codereview.chromium.org/12041098/diff/49021/tools/json_schema_compile... tools/json_schema_compiler/cpp_type_generator.py:51: self._namespace = namespace On 2013/02/05 00:47:46, kalman wrote: > And this logic shouldn't be necessary. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/12041098/66001
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/12041098/76001
Message was sent while issue was closed.
Change committed as 180845 |
