Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16462004:
Add optional schema compiler error messages
|
Created:
7 years, 6 months ago by Aaron Jacobs Modified:
7 years, 2 months ago CC:
chromium-reviews, evan.peterson.ep_gmail.com, jshumway Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd optional schema compiler error messages
Provides the ability to generate error messages within schema-compiled code for ease of debugging.
Error messages may be enabled by adding a 'generate_error_messages' property to json schema, e.g. "generate_error_messages": true
New sample: https://gist.github.com/Samusaaron3/32e36eb059a6911f3652
Old sample: https://gist.github.com/Samusaaron3/476bbc7114b90cfb0a0d
BUG=234834
Patch Set 1 #
Total comments: 10
Patch Set 2 : Clark's requests #
Total comments: 41
Patch Set 3 : Kalman's requests #
Total comments: 24
Patch Set 4 : Kalman's requests #
Total comments: 6
Messages
Total messages: 16 (0 generated)
could you post a gist of a sample of what the generated code looks like? https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:188: 'std::string* error) {') line this up with the "'" on the line above https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:516: c, move this up in the ( https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:539: 'const base::ListValue& args, std::string* error) {') line up with ( https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:620: c, put after ( https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:635: c, put after ( if it fits (everywhere)
Gist link added to description https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:188: 'std::string* error) {') On 2013/06/07 19:10:42, cduvall wrote: > line this up with the "'" on the line above Done. https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:516: c, On 2013/06/07 19:10:42, cduvall wrote: > move this up in the ( Done. https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:539: 'const base::ListValue& args, std::string* error) {') On 2013/06/07 19:10:42, cduvall wrote: > line up with ( Done. https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:620: c, On 2013/06/07 19:10:42, cduvall wrote: > put after ( Done. https://codereview.chromium.org/16462004/diff/1/tools/json_schema_compiler/cc... tools/json_schema_compiler/cc_generator.py:635: c, On 2013/06/07 19:10:42, cduvall wrote: > put after ( if it fits (everywhere) Done.
kalman: Schema compiler error messages you requested, let me know what you think.
awesome. The comments are all around a couple of themes really, and yeah, keep in mind that these are for the warnings shown to extension developers (that little red box on chrome:extensions when there are warnings), not Chrome developers. Also: (1) please add tests (the gist thing you posted would be a good start) (2) add this to manifest_types.json and start using it now :) https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:190: c.Sblock(' const base::Value& value, %(name)s* out) {') I looked at h_generator first so are making comments in here assuming you've read those first. Same deal though, make parameters a list, etc. Maybe this should actually be a method on cc_generator.py (I kinda wish these extended a base class, but only kind of; it's not really that complicated): def _GenerateParamsWithError(self, *params): if self._GenreateErrorMessages(): params = params[:] + ['std::string* error'] return ', '.join(params) see how that looks. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:206: self._GenerateError(c, instead of passing |c| in, make _GenerateError return a Code object and Concat it. c.Concat(self._GenerateError(...)) same for all other calls. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:208: '"unexpected type, got " << value.GetType()') value.GetType() returns an int but we should show a nice meaningful string. add a method into tools/json_schema_compiler/util.{h,cc} but see if one already exists in chrome somewhere. If it does maybe we should move it into value.h or something. That said I don't think this error message is actually that useful. We want to associate errors with keys, not with their values. I.e. if we have type: { id: Port, type: object } myObject: { properties: { myParam: { $ref: Port } } } and somebody tries to do like { myParam: 10 } we want to say "myParam": expecting object got int not unexpected type, got int that may require a bit of restructuring, since there won't always be a key, maybe it's a top-level type. the quickest way would probably be to pass the key name into here and if it's non-None show that, if it's None just show "expecting object got int". a nicer one would be to return the error somehow and have the caller decide how to show it, but in the generated code it may actually be too late to make this decision, if these already 'return false'. maybe we could have like, for all of these, type_populate_code, error = self._GenerateTypePopulate(....) if error: if self._ShouldGenerateErrorMessages(): type_populate_code.Concat(self._GenerateErrorMessage(...)) type_populate_code.Append('return false') c.Append(type_populate_code) but that may be impractical depending on how all of this is structured. in any case - same comment throughout this file, where appropriate. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:275: '"key not found: %(key)s"', '"%(key)s" is required' https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:568: '"missing non-optional value: %(value_var)s"') '"%s" is required' https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:620: '"expected %s, got " << %s->GetType()' % same comments re key name https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:674: .Append('}') same comments; and is there a way to avoid so much repetition here? https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:689: (c.Append(' return %(failure_value)s;') as well as saying what the key is, it would be nice to say what index the error was at; but that probably means cooperating with the C++ utility. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:708: '"unable to populate array"', yeah; bummer. maybe those methods need to be changed to take an error. ugh! oh well. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:949: def _GenerateError(self, c, location, body, indent = False): - return the code don't append to an argument - control the indentation level from the caller not here https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:956: """ assert self._ShouldGenerateErrors() https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:961: .Append(padding + ' std::stringstream ss;') just use string concatenation https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:963: ' << "\' at " << __FILE__ << ":" << (__LINE__ - 3);') this is all unlikely to be of interest to an extension developer. I think all this method really needs to take is |body|. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' just make it take an required parameter error, no optional parameter. there won't be much code to update. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:234: '%s* out%s);' % (classname, self._error_parameter)) perhaps: params = ['const base::Value& value', '%s out' % classname] if self._GenerateErrorMessages(): params.append('std::string* error') ... .Append('static bool Populate(%s);' % ', '.join(params)) which involves adding a method _GenerateErrorMessages() which would look up the compiler options. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:242: (classname, self._error_parameter)) likewise. maybe there is a nice way to share that logic here and above and in cc_generator. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:286: '%s* out%s);' % (classname, self._error_parameter)) ditto https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:346: 'const base::ListValue& args%s);' % self._error_parameter) ditto https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/model.py:54: - |generate_error_messages| if error messages should be generated make this part of the compiler options.
As a heads up, tomorrow is going to be my last day of work for quite a while (possibly until January), so I will not be able to get much more done on this CL unfortunately. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:190: c.Sblock(' const base::Value& value, %(name)s* out) {') On 2013/06/11 18:33:43, kalman wrote: > I looked at h_generator first so are making comments in here assuming you've > read those first. > > Same deal though, make parameters a list, etc. Maybe this should actually be a > method on cc_generator.py (I kinda wish these extended a base class, but only > kind of; it's not really that complicated): > > def _GenerateParamsWithError(self, *params): > if self._GenreateErrorMessages(): > params = params[:] + ['std::string* error'] > return ', '.join(params) > > see how that looks. Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:206: self._GenerateError(c, On 2013/06/11 18:33:43, kalman wrote: > instead of passing |c| in, make _GenerateError return a Code object and Concat > it. > > c.Concat(self._GenerateError(...)) > > same for all other calls. Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:208: '"unexpected type, got " << value.GetType()') On 2013/06/11 18:33:43, kalman wrote: > value.GetType() returns an int but we should show a nice meaningful string. add > a method into tools/json_schema_compiler/util.{h,cc} but see if one already > exists in chrome somewhere. If it does maybe we should move it into value.h or > something. > > That said I don't think this error message is actually that useful. We want to > associate errors with keys, not with their values. I.e. if we have > > type: { > id: Port, > type: object > } > > myObject: { > properties: { > myParam: { > $ref: Port > } > } > } > > and somebody tries to do like > > { myParam: 10 } > > we want to say > > "myParam": expecting object got int > > not > > unexpected type, got int > > that may require a bit of restructuring, since there won't always be a key, > maybe it's a top-level type. the quickest way would probably be to pass the key > name into here and if it's non-None show that, if it's None just show "expecting > object got int". > > a nicer one would be to return the error somehow and have the caller decide how > to show it, but in the generated code it may actually be too late to make this > decision, if these already 'return false'. maybe we could have like, for all of > these, > > type_populate_code, error = self._GenerateTypePopulate(....) > if error: > if self._ShouldGenerateErrorMessages(): > type_populate_code.Concat(self._GenerateErrorMessage(...)) > type_populate_code.Append('return false') > c.Append(type_populate_code) > > but that may be impractical depending on how all of this is structured. > > in any case - same comment throughout this file, where appropriate. Done (kinda) - I'm not completely sure how to get the javascript variable names from the schema compiler. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:275: '"key not found: %(key)s"', On 2013/06/11 18:33:43, kalman wrote: > '"%(key)s" is required' Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:568: '"missing non-optional value: %(value_var)s"') On 2013/06/11 18:33:43, kalman wrote: > '"%s" is required' Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:620: '"expected %s, got " << %s->GetType()' % On 2013/06/11 18:33:43, kalman wrote: > same comments re key name Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:674: .Append('}') On 2013/06/11 18:33:43, kalman wrote: > same comments; and is there a way to avoid so much repetition here? Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:949: def _GenerateError(self, c, location, body, indent = False): On 2013/06/11 18:33:43, kalman wrote: > - return the code don't append to an argument > - control the indentation level from the caller not here Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:961: .Append(padding + ' std::stringstream ss;') On 2013/06/11 18:33:43, kalman wrote: > just use string concatenation Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/cc_generator.py:963: ' << "\' at " << __FILE__ << ":" << (__LINE__ - 3);') On 2013/06/11 18:33:43, kalman wrote: > this is all unlikely to be of interest to an extension developer. I think all > this method really needs to take is |body|. Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/11 18:33:43, kalman wrote: > just make it take an required parameter error, no optional parameter. there > won't be much code to update. tools/json_schema_compiler/util is dependent on the function signature, and would need to change depending on the |compiler_options| https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:234: '%s* out%s);' % (classname, self._error_parameter)) On 2013/06/11 18:33:43, kalman wrote: > perhaps: > > params = ['const base::Value& value', '%s out' % classname] > if self._GenerateErrorMessages(): > params.append('std::string* error') > ... > .Append('static bool Populate(%s);' % ', '.join(params)) > > which involves adding a method _GenerateErrorMessages() which would look up the > compiler options. Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:242: (classname, self._error_parameter)) On 2013/06/11 18:33:43, kalman wrote: > likewise. maybe there is a nice way to share that logic here and above and in > cc_generator. Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:286: '%s* out%s);' % (classname, self._error_parameter)) On 2013/06/11 18:33:43, kalman wrote: > ditto Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:346: 'const base::ListValue& args%s);' % self._error_parameter) On 2013/06/11 18:33:43, kalman wrote: > ditto Done. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/model.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/model.py:54: - |generate_error_messages| if error messages should be generated On 2013/06/11 18:33:43, kalman wrote: > make this part of the compiler options. Done.
https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 02:00:37, Aaron Jacobs wrote: > On 2013/06/11 18:33:43, kalman wrote: > > just make it take an required parameter error, no optional parameter. there > > won't be much code to update. > > tools/json_schema_compiler/util is dependent on the function signature, and > would need to change depending on the |compiler_options| that's ok. Just pass through NULL or something in the case when we're not generating it.
https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 02:14:01, kalman wrote: > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > On 2013/06/11 18:33:43, kalman wrote: > > > just make it take an required parameter error, no optional parameter. there > > > won't be much code to update. > > > > tools/json_schema_compiler/util is dependent on the function signature, and > > would need to change depending on the |compiler_options| > > that's ok. Just pass through NULL or something in the case when we're not > generating it. While that will work for this situation, there is a larger issue - if I make the parameter required, all calls to |FromValue| or |Populate| anywhere within the chromium codebase will need to be changed to pass in a NULL argument.
https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 21:11:30, Aaron Jacobs wrote: > On 2013/06/14 02:14:01, kalman wrote: > > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > > On 2013/06/11 18:33:43, kalman wrote: > > > > just make it take an required parameter error, no optional parameter. > there > > > > won't be much code to update. > > > > > > tools/json_schema_compiler/util is dependent on the function signature, and > > > would need to change depending on the |compiler_options| > > > > that's ok. Just pass through NULL or something in the case when we're not > > generating it. > > While that will work for this situation, there is a larger issue - if I make the > parameter required, all calls to |FromValue| or |Populate| anywhere within the > chromium codebase will need to be changed to pass in a NULL argument. Why? Still generate the correct Populate, just in the no-error mode make it pass NULL.
https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 21:14:15, kalman wrote: > On 2013/06/14 21:11:30, Aaron Jacobs wrote: > > On 2013/06/14 02:14:01, kalman wrote: > > > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > > > On 2013/06/11 18:33:43, kalman wrote: > > > > > just make it take an required parameter error, no optional parameter. > > there > > > > > won't be much code to update. > > > > > > > > tools/json_schema_compiler/util is dependent on the function signature, > and > > > > would need to change depending on the |compiler_options| > > > > > > that's ok. Just pass through NULL or something in the case when we're not > > > generating it. > > > > While that will work for this situation, there is a larger issue - if I make > the > > parameter required, all calls to |FromValue| or |Populate| anywhere within the > > chromium codebase will need to be changed to pass in a NULL argument. > > Why? Still generate the correct Populate, just in the no-error mode make it pass > NULL. If the parameter is not optional, then the function signature between extensions with error messages and those without are different, which causes issues if they reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, which references tabs). In order to keep them compatible, either all calls need to have the parameter, all need to not have the parameter, or the parameter needs to be optional.
cool. yeah, most of those comments should be applied across the code, not just to where I made them. also - as I mentioned it would be great if you could apply this to manifest_types.json. https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 21:21:24, Aaron Jacobs wrote: > On 2013/06/14 21:14:15, kalman wrote: > > On 2013/06/14 21:11:30, Aaron Jacobs wrote: > > > On 2013/06/14 02:14:01, kalman wrote: > > > > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > > > > On 2013/06/11 18:33:43, kalman wrote: > > > > > > just make it take an required parameter error, no optional parameter. > > > there > > > > > > won't be much code to update. > > > > > > > > > > tools/json_schema_compiler/util is dependent on the function signature, > > and > > > > > would need to change depending on the |compiler_options| > > > > > > > > that's ok. Just pass through NULL or something in the case when we're not > > > > generating it. > > > > > > While that will work for this situation, there is a larger issue - if I make > > the > > > parameter required, all calls to |FromValue| or |Populate| anywhere within > the > > > chromium codebase will need to be changed to pass in a NULL argument. > > > > Why? Still generate the correct Populate, just in the no-error mode make it > pass > > NULL. > > If the parameter is not optional, then the function signature between extensions > with error messages and those without are different, which causes issues if they > reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, which > references tabs). In order to keep them compatible, either all calls need to > have the parameter, all need to not have the parameter, or the parameter needs > to be optional. I see. jyasskin@ is there an idiom for this? It seems wasteful to always generate two constructors. Is an optional parameter justified in this case? https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:206: 'json_schema_compiler::util::ValueTypeToString(value.GetType())')) can you use the same style here (and everywhere), where instead of c.Append(...) c.Append(...) c.Concat(...) there is (c.Append(..) .Append(...) .Concat(...)) and also: "unexpected type" -> "unexpected type choice, got ..." in lieu of actually doing this properly and saying "unexpected type, got $type but expecting one of [$types]", which I don't mind us not doing since choices is quite rare. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:212: 'json_schema_compiler::util::ValueTypeToString(value.GetType())', and also there are wrapper for all these in util_cc_helper.py, we should use that https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:269: c.Concat(self._GenerateError('"%%(key)s is required"', True)) let's standardise on something. single quotes - I think that looks better - so this should be (oh dear) '"\'%%(key)s\' is required"'. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:546: c.Concat(self._GenerateError('"%%(value_var)s is required"')) ditto, single quotes around the key in here https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:595: c.Concat(self._GenerateError('"\'%s\': expected %s, got " + ' ditto, and the inside-parens thing all through here https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:597: (src_var, self._type_helper.GetCppType(type_), src_var), True)) src_var isn't right, that's a c++ variable name. we want the key name in the schema. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:673: c.Concat(self._GenerateError('"unable to populate array"', True)) key can be known here, right? https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:773: '"\'%s\': expected std::string, got " + ' just "string" not "std::string" https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:917: .Append(padding + ' *error = ' + body + ';')) why do you need this padding logic? the caller can do it if necessary. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/h_generator.py:233: ['const base::Value& value', '%s* out' % classname])) for all of these, pass in a tuple not a list, cc_generator too https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/util.cc (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:89: out = "std::string"; just "string" https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:105: } 1. this can be a lot shorter if you just have 'return "null"' rather than assigning to |out|. 2. leave out the default case so that the compiler catches errors. instead, have NOTREACHED at the end and return ""
https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', std::string* error = NULL' On 2013/06/14 21:48:39, kalman wrote: > On 2013/06/14 21:21:24, Aaron Jacobs wrote: > > On 2013/06/14 21:14:15, kalman wrote: > > > On 2013/06/14 21:11:30, Aaron Jacobs wrote: > > > > On 2013/06/14 02:14:01, kalman wrote: > > > > > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > > > > > On 2013/06/11 18:33:43, kalman wrote: > > > > > > > just make it take an required parameter error, no optional > parameter. > > > > there > > > > > > > won't be much code to update. > > > > > > > > > > > > tools/json_schema_compiler/util is dependent on the function > signature, > > > and > > > > > > would need to change depending on the |compiler_options| > > > > > > > > > > that's ok. Just pass through NULL or something in the case when we're > not > > > > > generating it. > > > > > > > > While that will work for this situation, there is a larger issue - if I > make > > > the > > > > parameter required, all calls to |FromValue| or |Populate| anywhere within > > the > > > > chromium codebase will need to be changed to pass in a NULL argument. > > > > > > Why? Still generate the correct Populate, just in the no-error mode make it > > pass > > > NULL. > > > > If the parameter is not optional, then the function signature between > extensions > > with error messages and those without are different, which causes issues if > they > > reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, which > > references tabs). In order to keep them compatible, either all calls need to > > have the parameter, all need to not have the parameter, or the parameter needs > > to be optional. > > I see. jyasskin@ is there an idiom for this? It seems wasteful to always > generate two constructors. Is an optional parameter justified in this case? I see duplicate functions but not duplicate constructors. Lawrence Crowl argued for the current rule, that overloaded functions should be preferred over default arguments, and I don't think the reasoning is actually compelling, but for everything but constructors it's easy and efficient to just generate the inline forwarding function. For constructors, I'd use a default argument.
On 2013/06/14 21:58:47, Jeffrey Yasskin wrote: > https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... > File tools/json_schema_compiler/h_generator.py (right): > > https://codereview.chromium.org/16462004/diff/7001/tools/json_schema_compiler... > tools/json_schema_compiler/h_generator.py:31: self._error_parameter = ', > std::string* error = NULL' > On 2013/06/14 21:48:39, kalman wrote: > > On 2013/06/14 21:21:24, Aaron Jacobs wrote: > > > On 2013/06/14 21:14:15, kalman wrote: > > > > On 2013/06/14 21:11:30, Aaron Jacobs wrote: > > > > > On 2013/06/14 02:14:01, kalman wrote: > > > > > > On 2013/06/14 02:00:37, Aaron Jacobs wrote: > > > > > > > On 2013/06/11 18:33:43, kalman wrote: > > > > > > > > just make it take an required parameter error, no optional > > parameter. > > > > > there > > > > > > > > won't be much code to update. > > > > > > > > > > > > > > tools/json_schema_compiler/util is dependent on the function > > signature, > > > > and > > > > > > > would need to change depending on the |compiler_options| > > > > > > > > > > > > that's ok. Just pass through NULL or something in the case when we're > > not > > > > > > generating it. > > > > > > > > > > While that will work for this situation, there is a larger issue - if I > > make > > > > the > > > > > parameter required, all calls to |FromValue| or |Populate| anywhere > within > > > the > > > > > chromium codebase will need to be changed to pass in a NULL argument. > > > > > > > > Why? Still generate the correct Populate, just in the no-error mode make > it > > > pass > > > > NULL. > > > > > > If the parameter is not optional, then the function signature between > > extensions > > > with error messages and those without are different, which causes issues if > > they > > > reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, > which > > > references tabs). In order to keep them compatible, either all calls need to > > > have the parameter, all need to not have the parameter, or the parameter > needs > > > to be optional. > > > > I see. jyasskin@ is there an idiom for this? It seems wasteful to always > > generate two constructors. Is an optional parameter justified in this case? > > I see duplicate functions but not duplicate constructors. Lawrence Crowl argued > for the current rule, that overloaded functions should be preferred over default > arguments, and I don't think the reasoning is actually compelling, but for > everything but constructors it's easy and efficient to just generate the inline > forwarding function. For constructors, I'd use a default argument. Cool, thanks. I did actually mean "functions". I dunno where constructors came from.
https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:206: 'json_schema_compiler::util::ValueTypeToString(value.GetType())')) On 2013/06/14 21:48:39, kalman wrote: > can you use the same style here (and everywhere), where instead of > > c.Append(...) > c.Append(...) > c.Concat(...) > > there is > > (c.Append(..) > .Append(...) > .Concat(...)) > > and also: "unexpected type" -> "unexpected type choice, got ..." in lieu of > actually doing this properly and saying "unexpected type, got $type but > expecting one of [$types]", which I don't mind us not doing since choices is > quite rare. Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:212: 'json_schema_compiler::util::ValueTypeToString(value.GetType())', On 2013/06/14 21:48:39, kalman wrote: > and also there are wrapper for all these in util_cc_helper.py, we should use > that Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:269: c.Concat(self._GenerateError('"%%(key)s is required"', True)) On 2013/06/14 21:48:39, kalman wrote: > let's standardise on something. single quotes - I think that looks better - so > this should be (oh dear) '"\'%%(key)s\' is required"'. Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:546: c.Concat(self._GenerateError('"%%(value_var)s is required"')) On 2013/06/14 21:48:39, kalman wrote: > ditto, single quotes around the key in here Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:595: c.Concat(self._GenerateError('"\'%s\': expected %s, got " + ' On 2013/06/14 21:48:39, kalman wrote: > ditto, and the inside-parens thing all through here Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:597: (src_var, self._type_helper.GetCppType(type_), src_var), True)) On 2013/06/14 21:48:39, kalman wrote: > src_var isn't right, that's a c++ variable name. we want the key name in the > schema. Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:673: c.Concat(self._GenerateError('"unable to populate array"', True)) On 2013/06/14 21:48:39, kalman wrote: > key can be known here, right? Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:773: '"\'%s\': expected std::string, got " + ' On 2013/06/14 21:48:39, kalman wrote: > just "string" not "std::string" Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:917: .Append(padding + ' *error = ' + body + ';')) On 2013/06/14 21:48:39, kalman wrote: > why do you need this padding logic? the caller can do it if necessary. Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/h_generator.py:233: ['const base::Value& value', '%s* out' % classname])) On 2013/06/14 21:48:39, kalman wrote: > for all of these, pass in a tuple not a list, cc_generator too Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... File tools/json_schema_compiler/util.cc (right): https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:89: out = "std::string"; On 2013/06/14 21:48:39, kalman wrote: > just "string" Done. https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:105: } On 2013/06/14 21:48:39, kalman wrote: > 1. this can be a lot shorter if you just have 'return "null"' rather than > assigning to |out|. > 2. leave out the default case so that the compiler catches errors. instead, have > NOTREACHED at the end and return "" Done.
On 2013/06/15 00:47:17, Aaron Jacobs wrote: > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > File tools/json_schema_compiler/cc_generator.py (right): > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:206: > 'json_schema_compiler::util::ValueTypeToString(value.GetType())')) > On 2013/06/14 21:48:39, kalman wrote: > > can you use the same style here (and everywhere), where instead of > > > > c.Append(...) > > c.Append(...) > > c.Concat(...) > > > > there is > > > > (c.Append(..) > > .Append(...) > > .Concat(...)) > > > > and also: "unexpected type" -> "unexpected type choice, got ..." in lieu of > > actually doing this properly and saying "unexpected type, got $type but > > expecting one of [$types]", which I don't mind us not doing since choices is > > quite rare. > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:212: > 'json_schema_compiler::util::ValueTypeToString(value.GetType())', > On 2013/06/14 21:48:39, kalman wrote: > > and also there are wrapper for all these in util_cc_helper.py, we should use > > that > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:269: > c.Concat(self._GenerateError('"%%(key)s is required"', True)) > On 2013/06/14 21:48:39, kalman wrote: > > let's standardise on something. single quotes - I think that looks better - so > > this should be (oh dear) '"\'%%(key)s\' is required"'. > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:546: > c.Concat(self._GenerateError('"%%(value_var)s is required"')) > On 2013/06/14 21:48:39, kalman wrote: > > ditto, single quotes around the key in here > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:595: > c.Concat(self._GenerateError('"\'%s\': expected %s, got " + ' > On 2013/06/14 21:48:39, kalman wrote: > > ditto, and the inside-parens thing all through here > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:597: (src_var, > self._type_helper.GetCppType(type_), src_var), True)) > On 2013/06/14 21:48:39, kalman wrote: > > src_var isn't right, that's a c++ variable name. we want the key name in the > > schema. > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:673: > c.Concat(self._GenerateError('"unable to populate array"', True)) > On 2013/06/14 21:48:39, kalman wrote: > > key can be known here, right? > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:773: '"\'%s\': expected std::string, > got " + ' > On 2013/06/14 21:48:39, kalman wrote: > > just "string" not "std::string" > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/cc_generator.py:917: .Append(padding + ' *error = ' > + body + ';')) > On 2013/06/14 21:48:39, kalman wrote: > > why do you need this padding logic? the caller can do it if necessary. > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > File tools/json_schema_compiler/h_generator.py (right): > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/h_generator.py:233: ['const base::Value& value', '%s* > out' % classname])) > On 2013/06/14 21:48:39, kalman wrote: > > for all of these, pass in a tuple not a list, cc_generator too > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > File tools/json_schema_compiler/util.cc (right): > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/util.cc:89: out = "std::string"; > On 2013/06/14 21:48:39, kalman wrote: > > just "string" > > Done. > > https://codereview.chromium.org/16462004/diff/14001/tools/json_schema_compile... > tools/json_schema_compiler/util.cc:105: } > On 2013/06/14 21:48:39, kalman wrote: > > 1. this can be a lot shorter if you just have 'return "null"' rather than > > assigning to |out|. > > 2. leave out the default case so that the compiler catches errors. instead, > have > > NOTREACHED at the end and return "" > > Done. And with this, I conclude my work on Chromium (for the time being). Hopefully it isn't too much of a pain for someone else to take over.
thanks. lg but needs tests! Jared/Evan is one of you able to triage this patch to somebody on your end? It doesn't have to be done right away but I don't want it to disappear. Basically there are those few last comments + it needs tests :\ not particularly glamorous, sorry. Tests are important. I have no idea if this stuff even compiles, though presumably Aaron does. https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... File tools/json_schema_compiler/cc_generator.py (right): https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:669: '"unable to populate array \'%%(array_key)s\'"')) better this explicitly be parent_key https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/cc_generator.py:919: return ', '.join(str(p) for p in params) here and below should be something more like: if self._generate_error_messages: params = list(params) + ['std::string* error'] ... https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... File tools/json_schema_compiler/h_generator.py (right): https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/h_generator.py:403: return ', '.join(str(p) for p in params) see list thing again https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... File tools/json_schema_compiler/util.cc (right): https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:80: return "int"; "integer" https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:82: return "double"; "number" https://codereview.chromium.org/16462004/diff/30001/tools/json_schema_compile... tools/json_schema_compiler/util.cc:92: ; no default case |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
