Issue 29743581: Issue 6552 - Support arbitrary manifest values (Closed)
|
Created:
April 5, 2018, 10:21 a.m. by tlucas Modified:
April 19, 2018, 3:03 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/buildtools/file/a3db4a1a49e8 Visibility:
Public. |
DescriptionIssue 6552 - Support arbitrary manifest values
Patch Set 1 #Patch Set 2 : Issue 6552 - Support arbitrary manifest values #
Total comments: 14
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 22
Patch Set 5 : NOCHANGE rebase against current master (30b4e987f025) #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 17
Patch Set 8 : #Patch Set 9 : #
Total comments: 2
MessagesTotal messages: 26
Patch Set 1 * Support externally_connectable in manifest.json
Just an idea, but I wonder whether it begins to make sense to implement a more
generic mechanism rather than adding some explicit boilerplate every time
AdBlock (or ourselves) want to use a new manifest key that doesn't need any
processing but just needs to be passed through to the generated manifest.json
(e.g. externally_connectable, content_security_policy,
web_accessible_resources).
We could add a new section (called "manifest") in the metadata.* file which can
contain arbitrary keys in following format:
foo = foo
bar.baz[0] = bar
Which then get added to the generated mainfest.json like this:
"foo": "foo",
"bar": {
"baz": [
"bar"
]
}
What do you think?
On 2018/04/05 19:14:03, Sebastian Noack wrote:
> Just an idea, but I wonder whether it begins to make sense to implement a more
> generic mechanism rather than adding some explicit boilerplate every time
> AdBlock (or ourselves) want to use a new manifest key that doesn't need any
> processing but just needs to be passed through to the generated manifest.json
> (e.g. externally_connectable, content_security_policy,
> web_accessible_resources).
>
> We could add a new section (called "manifest") in the metadata.* file which
can
> contain arbitrary keys in following format:
>
> foo = foo
> bar.baz[0] = bar
>
> Which then get added to the generated mainfest.json like this:
>
> "foo": "foo",
> "bar": {
> "baz": [
> "bar"
> ]
> }
>
> What do you think?
Actually, that's exactly what i was thinking before implementing this (i kinda
had that idea the last time i changed something similiar)
Love the idea, will do! ( - in case we agree on "this is the time for it")
Patch Set 2 * Implemented support for arbitrary values in a metadata's `manifest`-section * Modified tests to verify the new feature Hey guys. I gave it a try and implemented this new feature. In theory, we might be able to get rid of some switches in 'templates/manifest.json.tmpl', but i wanted to know your thoughts before that.
https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:210: return v * I cannot think of any relevant manifest key where "true", "false", etc. or anything that parses as a number would be to be interpreted as a string. So I would go with more implicit semantics in favor of less obscure syntax. * In particular the distinction between integer and float is superfluous since all numbers in (JavaScript and) JSON are treated as double-precision floating point numbers. However, Python's "json" module is not compliant with the JSON standard as it treat numbers without floating point character as integers. * There are typed ConfigParser.get*() methods. So how about this: try: self.getint(...) except ValueError: try: self.getfloat(...) except ValueError: try: self.getboolean(...) except ValueError: self.get(...) https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:216: current = target.setdefault(arr[0], {}) You might want to use an OrderedDict to make sure the keys remain in the same order as in the JSON file, to preserve the readability of the generated mainfest.json, but also to make sure that the expected output for the tests don't depend on the behavior of Python internals. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:220: result = {} Same here. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:226: value = v.split() if is_array else v It seems the logic here can be simplified: if k.endswith('[]'): k = k[:-2] v = v.split() setdefault_recursive(result, k.split('.') + [value]) https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chrome File tests/metadata.chrome (left): https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chro... tests/metadata.chrome:17: webAccessible = images/*.png Please update https://issues.adblockplus.org/ticket/6567 with integration notes about the respective changes to the production metadata.* files. https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chro... tests/metadata.chrome:16: permissions[] = tabs I think its more readable if we put all items on a new align and align them.
Patch Set 3 * Use OrderedDict in parsing * reindented metadata.chrome * replied to Sebastian's comments Hey Sebastian, thanks for your feedback! Please find the discussion below. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:210: return v On 2018/04/12 12:19:23, Sebastian Noack wrote: > * I cannot think of any relevant manifest key where "true", "false", etc. or > anything that parses as a number would be to be interpreted as a string. So I > would go with more implicit semantics in favor of less obscure syntax. > * In particular the distinction between integer and float is superfluous since > all numbers in (JavaScript and) JSON are treated as double-precision floating > point numbers. However, Python's "json" module is not compliant with the JSON > standard as it treat numbers without floating point character as integers. > * There are typed ConfigParser.get*() methods. > > So how about this: > > try: > self.getint(...) > except ValueError: > try: > self.getfloat(...) > except ValueError: > try: > self.getboolean(...) > except ValueError: > self.get(...) I do not fully oppose your thoughts, but what about the following examples: * an extension renders it's version via this machinery into the manifest.json, where the version only consists of 2 digits (e.g. 1.3) -> the version would get returned as a float, while at least Chrome expects this value to be a string (FWIW, this currently is the case for our "compat"-key, which also might endup in [manifest]) * Arrays can not be handled by the typed get*() methods, so all arrays would be restricted to contain strings only If we agree that those are justifiable restrictions, i will change the code accordingly - however, i'm currently thinking that those restrictions are not necessary. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:216: current = target.setdefault(arr[0], {}) On 2018/04/12 12:19:23, Sebastian Noack wrote: > You might want to use an OrderedDict to make sure the keys remain in the same > order as in the JSON file, to preserve the readability of the generated > mainfest.json, but also to make sure that the expected output for the tests > don't depend on the behavior of Python internals. Good point, done. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:220: result = {} On 2018/04/12 12:19:23, Sebastian Noack wrote: > Same here. See above, done. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:226: value = v.split() if is_array else v On 2018/04/12 12:19:23, Sebastian Noack wrote: > It seems the logic here can be simplified: > > if k.endswith('[]'): > k = k[:-2] > v = v.split() > > setdefault_recursive(result, k.split('.') + [value]) Done. https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chrome File tests/metadata.chrome (left): https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chro... tests/metadata.chrome:17: webAccessible = images/*.png On 2018/04/12 12:19:24, Sebastian Noack wrote: > Please update https://issues.adblockplus.org/ticket/6567 with integration notes > about the respective changes to the production metadata.* files. Will do, as soon as we sorted out, what actually needs to be changed for this feature. https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29743581/diff/29749559/tests/metadata.chro... tests/metadata.chrome:16: permissions[] = tabs On 2018/04/12 12:19:23, Sebastian Noack wrote: > I think its more readable if we put all items on a new align and align them. Done.
https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:210: return v On 2018/04/12 13:47:39, tlucas wrote: > On 2018/04/12 12:19:23, Sebastian Noack wrote: > > * I cannot think of any relevant manifest key where "true", "false", etc. or > > anything that parses as a number would be to be interpreted as a string. So I > > would go with more implicit semantics in favor of less obscure syntax. > > * In particular the distinction between integer and float is superfluous since > > all numbers in (JavaScript and) JSON are treated as double-precision floating > > point numbers. However, Python's "json" module is not compliant with the JSON > > standard as it treat numbers without floating point character as integers. > > * There are typed ConfigParser.get*() methods. > > > > So how about this: > > > > try: > > self.getint(...) > > except ValueError: > > try: > > self.getfloat(...) > > except ValueError: > > try: > > self.getboolean(...) > > except ValueError: > > self.get(...) > > I do not fully oppose your thoughts, but what about the following examples: > > * an extension renders it's version via this machinery into the manifest.json, > where the version only consists of 2 digits (e.g. 1.3) -> the version would get > returned as a float, while at least Chrome expects this value to be a string > (FWIW, this currently is the case for our "compat"-key, which also might endup > in [manifest]) I was thinking about the version number, but its not meant to be set through the "manifest" section as it needs some processing (for the build number in the development builds). Similar for the version numbers in the "compat" section. But I might see a case for a more explicit format, if you are still convinced it makes sense. However, I'm not too happy with the particular format you chose: * Appending \X to the end of the value is rather obscure. When I first looked at the test configuration (before I looked at the implementation), I was sure that this was a typo. :D * Still I don't think it makes sense to distinguish between int and float. Maybe we don't even have to distinguish between numbers and bool (dependent on the format we go with). So (if we go with a strictly typed format), here are two suggestions for an IMO more appropriate annotation format: foo = number:42 foo = number:4.2 foo = bool:true foo# = 42 foo# = 4.2 foo# = true foo[#] = 42 24 > * Arrays can not be handled by the typed get*() methods, so all arrays would be > restricted to contain strings only Ah right, never mind then. I didn't consider that we cannot use it for nested values. Still I'm not sure if using distutils here is appropriate. Simply supporting "true" and "false" (no variations) should be good enough anyway.
https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... File templates/manifest.json.tmpl (right): https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... templates/manifest.json.tmpl:120: {%- endif %} I just noticed, so we still have to add each supported key explicitly to the template? Doesn't that defeat the purpose of the whole change, i.e. allowing arbitrary keys?
On 2018/04/12 14:39:29, Sebastian Noack wrote: > https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py > File chainedconfigparser.py (right): > > https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... > chainedconfigparser.py:210: return v > On 2018/04/12 13:47:39, tlucas wrote: > > On 2018/04/12 12:19:23, Sebastian Noack wrote: > > > * I cannot think of any relevant manifest key where "true", "false", etc. or > > > anything that parses as a number would be to be interpreted as a string. So > I > > > would go with more implicit semantics in favor of less obscure syntax. > > > * In particular the distinction between integer and float is superfluous > since > > > all numbers in (JavaScript and) JSON are treated as double-precision > floating > > > point numbers. However, Python's "json" module is not compliant with the > JSON > > > standard as it treat numbers without floating point character as integers. > > > * There are typed ConfigParser.get*() methods. > > > > > > So how about this: > > > > > > try: > > > self.getint(...) > > > except ValueError: > > > try: > > > self.getfloat(...) > > > except ValueError: > > > try: > > > self.getboolean(...) > > > except ValueError: > > > self.get(...) > > > > I do not fully oppose your thoughts, but what about the following examples: > > > > * an extension renders it's version via this machinery into the > manifest.json, > > where the version only consists of 2 digits (e.g. 1.3) -> the version would > get > > returned as a float, while at least Chrome expects this value to be a string > > (FWIW, this currently is the case for our "compat"-key, which also might endup > > in [manifest]) > > I was thinking about the version number, but its not meant to be set through the > "manifest" section as it needs some processing (for the build number in the > development builds). Similar for the version numbers in the "compat" section. > But I might see a case for a more explicit format, if you are still convinced it > makes sense. However, I'm not too happy with the particular format you chose: > > * Appending \X to the end of the value is rather obscure. When I first looked at > the test configuration (before I looked at the implementation), I was sure that > this was a typo. :D > * Still I don't think it makes sense to distinguish between int and float. Maybe > we don't even have to distinguish between numbers and bool (dependent on the > format we go with). > > So (if we go with a strictly typed format), here are two suggestions for an IMO > more appropriate annotation format: > > foo = number:42 > foo = number:4.2 > foo = bool:true > > foo# = 42 > foo# = 4.2 > foo# = true > foo[#] = > 42 > 24 > > > * Arrays can not be handled by the typed get*() methods, so all arrays would > be > > restricted to contain strings only > > Ah right, never mind then. I didn't consider that we cannot use it for nested > values. Still I'm not sure if using distutils here is appropriate. Simply > supporting "true" and "false" (no variations) should be good enough anyway. Yes, i do think strictly typing makes sense ;) (FWIW, at least my own extension-in-dev would profit from that). I'd go with suggestion one, being the most explicit annotation - that way we don't have to deal with exceptions and IF an exception occurs, then most likely because the dev editing metata.* wanted a boolean and gave something like trve (wich obviously is an error). (as a side effect, we'd also be able have arrays of mixed types) I also agree to stick to [true|false] as valid values for booleans, so i'll get rid of distutils here. I wouldn't mind not distinguishing between float and int, but i'll at least cut off the fraction if it would be 0 (so that e.g. manifest_version doesn't render to 2.0 when 2 is given) What do you think? Did we miss anything? If we agree, i'll update the code and the ticket accordingly then. https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... File templates/manifest.json.tmpl (right): https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... templates/manifest.json.tmpl:120: {%- endif %} On 2018/04/12 15:10:49, Sebastian Noack wrote: > I just noticed, so we still have to add each supported key explicitly to the > template? Doesn't that defeat the purpose of the whole change, i.e. allowing > arbitrary keys? Uh. good catch. on it.
On 2018/04/12 15:29:30, tlucas wrote:
> I wouldn't mind not distinguishing between float and int, but i'll at least
cut
> off the fraction if it would be 0 (so that e.g. manifest_version doesn't
render
> to 2.0 when 2 is given)
I agree (assuming that is what you meant), to preserve the floating point
representation given in the medatadata.* file when generating the manifest.json.
So essentially I would parse values like that:
if value.startswith('number:'):
value = value.split(':', 1)[1]
try:
value = int(value)
except ValueError:
value = float(value)
elif value == 'bool:true':
value = True
elif value == 'bool:false':
value = False
On 2018/04/12 16:08:42, Sebastian Noack wrote:
> On 2018/04/12 15:29:30, tlucas wrote:
> > I wouldn't mind not distinguishing between float and int, but i'll at least
> cut
> > off the fraction if it would be 0 (so that e.g. manifest_version doesn't
> render
> > to 2.0 when 2 is given)
>
> I agree (assuming that is what you meant), to preserve the floating point
> representation given in the medatadata.* file when generating the
manifest.json.
> So essentially I would parse values like that:
>
>
> if value.startswith('number:'):
> value = value.split(':', 1)[1]
> try:
> value = int(value)
> except ValueError:
> value = float(value)
> elif value == 'bool:true':
> value = True
> elif value == 'bool:false':
> value = False
Exactly - i'll implement the changes then, thank you!
Patch Set 4 * Made strictly typing json values less obscure * Removed explicit reference of arbitrary values in manifest.json.tmpl * Purged trailing whitespaces from all test data. https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser... chainedconfigparser.py:210: return v On 2018/04/12 14:39:29, Sebastian Noack wrote: > On 2018/04/12 13:47:39, tlucas wrote: > > On 2018/04/12 12:19:23, Sebastian Noack wrote: > > > * I cannot think of any relevant manifest key where "true", "false", etc. or > > > anything that parses as a number would be to be interpreted as a string. So > I > > > would go with more implicit semantics in favor of less obscure syntax. > > > * In particular the distinction between integer and float is superfluous > since > > > all numbers in (JavaScript and) JSON are treated as double-precision > floating > > > point numbers. However, Python's "json" module is not compliant with the > JSON > > > standard as it treat numbers without floating point character as integers. > > > * There are typed ConfigParser.get*() methods. > > > > > > So how about this: > > > > > > try: > > > self.getint(...) > > > except ValueError: > > > try: > > > self.getfloat(...) > > > except ValueError: > > > try: > > > self.getboolean(...) > > > except ValueError: > > > self.get(...) > > > > I do not fully oppose your thoughts, but what about the following examples: > > > > * an extension renders it's version via this machinery into the > manifest.json, > > where the version only consists of 2 digits (e.g. 1.3) -> the version would > get > > returned as a float, while at least Chrome expects this value to be a string > > (FWIW, this currently is the case for our "compat"-key, which also might endup > > in [manifest]) > > I was thinking about the version number, but its not meant to be set through the > "manifest" section as it needs some processing (for the build number in the > development builds). Similar for the version numbers in the "compat" section. > But I might see a case for a more explicit format, if you are still convinced it > makes sense. However, I'm not too happy with the particular format you chose: > > * Appending \X to the end of the value is rather obscure. When I first looked at > the test configuration (before I looked at the implementation), I was sure that > this was a typo. :D > * Still I don't think it makes sense to distinguish between int and float. Maybe > we don't even have to distinguish between numbers and bool (dependent on the > format we go with). > > So (if we go with a strictly typed format), here are two suggestions for an IMO > more appropriate annotation format: > > foo = number:42 > foo = number:4.2 > foo = bool:true > > foo# = 42 > foo# = 4.2 > foo# = true > foo[#] = > 42 > 24 > > > * Arrays can not be handled by the typed get*() methods, so all arrays would > be > > restricted to contain strings only > > Ah right, never mind then. I didn't consider that we cannot use it for nested > values. Still I'm not sure if using distutils here is appropriate. Simply > supporting "true" and "false" (no variations) should be good enough anyway. Done. https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... File templates/manifest.json.tmpl (right): https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.... templates/manifest.json.tmpl:120: {%- endif %} On 2018/04/12 15:29:30, tlucas wrote: > On 2018/04/12 15:10:49, Sebastian Noack wrote: > > I just noticed, so we still have to add each supported key explicitly to the > > template? Doesn't that defeat the purpose of the whole change, i.e. allowing > > arbitrary keys? > > Uh. good catch. on it. Done.
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:161: def as_json_object(self, section): Actually there isn't anything specific to JSON in this function. It just returns a dictionary which then is converted to JSON in the template. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:171: * An option's key must end with '[]', to mark this option as an array Nit: Please make all bullet point end with punctation, for consistence. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:171: * An option's key must end with '[]', to mark this option as an array I just noticed, we could identify arrays by the presence of newlines, rather than by adding [] to the key: foo = just a string foo = 1st item of array 2nd item of array foo = array with only one item What do you think? https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:228: v = v.split() We might want to strip by newlines, so that array items can have spaces. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:230: setdefault_recursive(result, k.split('.') + [v]) It seems weird/unnecessary to merge the key segments and value into one list. Any reason to not pass them as separate arguments? https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#n... packagerChrome.py:104: # Generate templatedataa which does not need special processing Typo: templatedataa -> templatedata https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... File templates/manifest.json.tmpl (left): https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... templates/manifest.json.tmpl:73: "icons": {{icons|json}}, I guess content_security_policy can also be handled generically now?
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:161: def as_json_object(self, section): On 2018/04/14 00:33:07, Sebastian Noack wrote: > Actually there isn't anything specific to JSON in this function. It just returns > a dictionary which then is converted to JSON in the template. You are right - what would you suggest? How about merge_to_nested()? https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:171: * An option's key must end with '[]', to mark this option as an array On 2018/04/14 00:33:07, Sebastian Noack wrote: > I just noticed, we could identify arrays by the presence of newlines, rather > than by adding [] to the key: > > foo = just a string > > foo = > 1st item of array > 2nd item of array > > foo = > array with only one item > > What do you think? The bullet points: ack The newlines: Nice idea, will do https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:228: v = v.split() On 2018/04/14 00:33:06, Sebastian Noack wrote: > We might want to strip by newlines, so that array items can have spaces. Acknowledged. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:230: setdefault_recursive(result, k.split('.') + [v]) On 2018/04/14 00:33:06, Sebastian Noack wrote: > It seems weird/unnecessary to merge the key segments and value into one list. > Any reason to not pass them as separate arguments? Not at all, will change. https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#n... packagerChrome.py:104: # Generate templatedataa which does not need special processing On 2018/04/14 00:33:07, Sebastian Noack wrote: > Typo: templatedataa -> templatedata Acknowledged. https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... File templates/manifest.json.tmpl (left): https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... templates/manifest.json.tmpl:73: "icons": {{icons|json}}, On 2018/04/14 00:33:07, Sebastian Noack wrote: > I guess content_security_policy can also be handled generically now? Yes - same applies to e.g. "managedStorageSchema". I will compare the buildtools' metadata.* with the adblockpluschrome's metadata.*, to find other keys which can be added generically now. (just wondering, did you put this note under "icons" on purpose?)
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:161: def as_json_object(self, section): On 2018/04/14 08:36:23, tlucas wrote: > On 2018/04/14 00:33:07, Sebastian Noack wrote: > > Actually there isn't anything specific to JSON in this function. It just > returns > > a dictionary which then is converted to JSON in the template. > > You are right - what would you suggest? How about merge_to_nested()? Maybe section_as_dict()? https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... File templates/manifest.json.tmpl (left): https://codereview.adblockplus.org/29743581/diff/29751570/templates/manifest.... templates/manifest.json.tmpl:73: "icons": {{icons|json}}, On 2018/04/14 08:36:24, tlucas wrote: > On 2018/04/14 00:33:07, Sebastian Noack wrote: > > I guess content_security_policy can also be handled generically now? > > Yes - same applies to e.g. "managedStorageSchema". > I will compare the buildtools' metadata.* with the adblockpluschrome's > metadata.*, to find other keys which can be added generically now. Yeah, storage.managed_scheme as well. I think that's it. As far as I can tell all the other keys involve some processing. > (just wondering, did you put this note under "icons" on purpose?) When looking at the unified diff, content_Security_policy wasn't in the context and this was the top most line I could comment on. I just noticed when looking at side-by-side diffs, I can expand the whole file.
https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#n... packagerChrome.py:140: manifest = json.dumps(data, sort_keys=True, indent=2) If we inject the generic keys here, we don't have to touch the template at all: data.update(metadata.as_json_object('manifest')) Or even better, if we pass "data" to "metadata.as_json_object()" (or whatever we end up calling this function), and merge the keys into the existing document, adding foo.x through the "metadata" section would work correctly even if foo.y is already generated by other means. Also I just noticed the sort_keys=True here. So I wonder if there is even a point to use OrderedDict?
Patch Set 6 * Use newlines as array definers * Removed redundant "OrderedDict" * Merging rendered manifest and arbitrary options, instead of rendering arbitrary options * cleaned up manifest.json.tmpl * Addressed further comments. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:161: def as_json_object(self, section): On 2018/04/14 08:54:26, Sebastian Noack wrote: > On 2018/04/14 08:36:23, tlucas wrote: > > On 2018/04/14 00:33:07, Sebastian Noack wrote: > > > Actually there isn't anything specific to JSON in this function. It just > > returns > > > a dictionary which then is converted to JSON in the template. > > > > You are right - what would you suggest? How about merge_to_nested()? > > Maybe section_as_dict()? Done. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:171: * An option's key must end with '[]', to mark this option as an array On 2018/04/14 08:36:23, tlucas wrote: > On 2018/04/14 00:33:07, Sebastian Noack wrote: > > I just noticed, we could identify arrays by the presence of newlines, rather > > than by adding [] to the key: > > > > foo = just a string > > > > foo = > > 1st item of array > > 2nd item of array > > > > foo = > > array with only one item > > > > What do you think? > > The bullet points: ack > > The newlines: Nice idea, will do Done. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:228: v = v.split() On 2018/04/14 08:36:23, tlucas wrote: > On 2018/04/14 00:33:06, Sebastian Noack wrote: > > We might want to strip by newlines, so that array items can have spaces. > > Acknowledged. Done. https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser... chainedconfigparser.py:230: setdefault_recursive(result, k.split('.') + [v]) On 2018/04/14 08:36:23, tlucas wrote: > On 2018/04/14 00:33:06, Sebastian Noack wrote: > > It seems weird/unnecessary to merge the key segments and value into one list. > > Any reason to not pass them as separate arguments? > > Not at all, will change. Done. https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#n... packagerChrome.py:104: # Generate templatedataa which does not need special processing On 2018/04/14 08:36:23, tlucas wrote: > On 2018/04/14 00:33:07, Sebastian Noack wrote: > > Typo: templatedataa -> templatedata > > Acknowledged. Done. (Removed it completely) https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#n... packagerChrome.py:140: manifest = json.dumps(data, sort_keys=True, indent=2) On 2018/04/14 09:08:39, Sebastian Noack wrote: > If we inject the generic keys here, we don't have to touch the template at all: > > data.update(metadata.as_json_object('manifest')) > > Or even better, if we pass "data" to "metadata.as_json_object()" (or whatever we > end up calling this function), and merge the keys into the existing document, > adding foo.x through the "metadata" section would work correctly even if foo.y > is already generated by other means. > > Also I just noticed the sort_keys=True here. So I wonder if there is even a > point to use OrderedDict? OrderedDict is indeed not necessary, removed it. The rest is also pretty neat, done!
https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:203: return [parse_values(x) for x in v] Checking for lists and recursively calling this function is unnecessary. The logic is simpler if you just move it where you check for the newline: if '\n' in v: ... = [parse_value(x) for x in v.splitlines() if x] else: ... = parse_value(v) https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:208: v = int(v) Nit: I wouldn't reassign v, but just return the parsed values right away. (Then you can also turn the "elif"s below into "if"s.) https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:231: setdefault_recursive(result, k.split('.'), v) The recursion here is unnecessary: parents = k.split('.') tail = parents.pop() current = result for name in parents: current = current.setdefault(name, {}) current[tail] = ... https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:233: result.update(additional) Well, the point of passing the generated part of the document into this function was to properly (deep) merge it with the content from the "manifest" section. So essentially this line should not exist, neither should the line that creates the new dictionary above (and assigns it to "result"). Instead you should just add stuff to the dictionary passed to this function. Here is a realistic example where this would be useful: The browser_action sub-document is generated like this (copied from the actual build): "browser_action": { "default_icon": { "16": "icons/abp-16.png", "19": "icons/abp-19.png", "20": "icons/abp-20.png", "32": "icons/abp-32.png", "38": "icons/abp-38.png", "40": "icons/abp-40.png" }, "default_title": "__MSG_name__" }, Now somebody wants to use the browser_action.browser_style property and could just add it to the "manifest" section and it gets added to the generated output. https://codereview.adblockplus.org/29743581/diff/29755749/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29743581/diff/29755749/tests/metadata.chro... tests/metadata.chrome:6: ext/common.js Nit: Two space indentation everywhere but in Python code (see our coding style).
Patch Set 7 * Removed redundant recursion * Properly deep merge into given dictionary * Reindentend the edited metadata.* files * Let parse_values() return immediately https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:203: return [parse_values(x) for x in v] On 2018/04/18 15:59:00, Sebastian Noack wrote: > Checking for lists and recursively calling this function is unnecessary. The > logic is simpler if you just move it where you check for the newline: > > if '\n' in v: > ... = [parse_value(x) for x in v.splitlines() if x] > else: > ... = parse_value(v) Done. https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:208: v = int(v) On 2018/04/18 15:58:59, Sebastian Noack wrote: > Nit: I wouldn't reassign v, but just return the parsed values right away. (Then > you can also turn the "elif"s below into "if"s.) Done. https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:231: setdefault_recursive(result, k.split('.'), v) On 2018/04/18 15:59:00, Sebastian Noack wrote: > The recursion here is unnecessary: > > parents = k.split('.') > tail = parents.pop() > current = result > for name in parents: > current = current.setdefault(name, {}) > current[tail] = ... Done. https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser... chainedconfigparser.py:233: result.update(additional) On 2018/04/18 15:58:59, Sebastian Noack wrote: > Well, the point of passing the generated part of the document into this function > was to properly (deep) merge it with the content from the "manifest" section. > > So essentially this line should not exist, neither should the line that creates > the new dictionary above (and assigns it to "result"). Instead you should just > add stuff to the dictionary passed to this function. > > Here is a realistic example where this would be useful: The browser_action > sub-document is generated like this (copied from the actual build): > > "browser_action": { > "default_icon": { > "16": "icons/abp-16.png", > "19": "icons/abp-19.png", > "20": "icons/abp-20.png", > "32": "icons/abp-32.png", > "38": "icons/abp-38.png", > "40": "icons/abp-40.png" > }, > "default_title": "__MSG_name__" > }, > > Now somebody wants to use the browser_action.browser_style property and could > just add it to the "manifest" section and it gets added to the generated output. Done. https://codereview.adblockplus.org/29743581/diff/29755749/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29743581/diff/29755749/tests/metadata.chro... tests/metadata.chrome:6: ext/common.js On 2018/04/18 15:59:01, Sebastian Noack wrote: > Nit: Two space indentation everywhere but in Python code (see our coding style). Done.
https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:171: will define the option's value as an array. Nit: Talking about "arrays" made sense initially when we used JSON terminology here. But since we no longer do, as this function is agnostic of JSON, we also might want to change "array" to "list" (Python). https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:185: asd "bar": { Nit: For consistency, we might want to use two space indentation on the left-hand side here as well. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:200: def parse_values(v): Nit; since this function is only called with a single value now, we might want to use singular: "parse_values" => "parse_value" https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:202: return [parse_values(x) for x in v] This code path can be removed now, since we no longer this function with a list. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:216: data = self.items(section) Nit: this variable is only used once, perhaps inline it below? https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:219: Nit: Is the blank line here intended? It looks weird, I don't recall seeing blank lines after a loop header anywhere else in our code. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:231: return base Nit: Returning the dictionary seems redundant. It's the same object passed in by the calling code, hence the calling code already has it. You merely can chain the function call in a single line, but therefore you have one more line here. On the other hand, if this function doesn't return it there is no potential confusion about the object being modified in-place. However, then we might want to rename this function to add_section_to_dict() or serialize_section().
Patch Set 8 * Addressed Sebastian's comments * Discussion about section absence handling, method name / return https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:171: will define the option's value as an array. On 2018/04/19 10:50:56, Sebastian Noack wrote: > Nit: Talking about "arrays" made sense initially when we used JSON terminology > here. But since we no longer do, as this function is agnostic of JSON, we also > might want to change "array" to "list" (Python). Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:185: asd "bar": { On 2018/04/19 10:50:57, Sebastian Noack wrote: > Nit: For consistency, we might want to use two space indentation on the > left-hand side here as well. Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:200: def parse_values(v): On 2018/04/19 10:50:57, Sebastian Noack wrote: > Nit; since this function is only called with a single value now, we might want > to use singular: "parse_values" => "parse_value" Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:202: return [parse_values(x) for x in v] On 2018/04/19 10:50:57, Sebastian Noack wrote: > This code path can be removed now, since we no longer this function with a list. Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:216: data = self.items(section) On 2018/04/19 10:50:56, Sebastian Noack wrote: > Nit: this variable is only used once, perhaps inline it below? Done. One note: Not catching a NoSectionError here would explicitly force [manifest] to be present in the metadata.*, even if it's empty. I might check for presence first, what do you think? https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:219: On 2018/04/19 10:50:58, Sebastian Noack wrote: > Nit: Is the blank line here intended? It looks weird, I don't recall seeing > blank lines after a loop header anywhere else in our code. Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:231: return base On 2018/04/19 10:50:57, Sebastian Noack wrote: > Nit: Returning the dictionary seems redundant. It's the same object passed in by > the calling code, hence the calling code already has it. You merely can chain > the function call in a single line, but therefore you have one more line here. > On the other hand, if this function doesn't return it there is no potential > confusion about the object being modified in-place. However, then we might want > to rename this function to add_section_to_dict() or serialize_section(). I wouldn't mind either solution - but we also might want to rename the method, when we're not returning anything anymore into e.g. merge_section_into(). What's your favorite?
https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:231: return base On 2018/04/19 11:08:14, tlucas wrote: > On 2018/04/19 10:50:57, Sebastian Noack wrote: > > Nit: Returning the dictionary seems redundant. It's the same object passed in > by > > the calling code, hence the calling code already has it. You merely can chain > > the function call in a single line, but therefore you have one more line here. > > On the other hand, if this function doesn't return it there is no potential > > confusion about the object being modified in-place. However, then we might > want > > to rename this function to add_section_to_dict() or serialize_section(). > > I wouldn't mind either solution - but we also might want to rename the method, > when we're not returning anything anymore into e.g. merge_section_into(). > > What's your favorite? That is exactly what I said. My suggestions above were add_section_to_dict() and serialize_section(); merge_section_into() is good to.
Patch Set 9 * Handle absence of the given section * Rename the new method to imply the presence check https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:216: data = self.items(section) On 2018/04/19 11:08:14, tlucas wrote: > On 2018/04/19 10:50:56, Sebastian Noack wrote: > > Nit: this variable is only used once, perhaps inline it below? > > Done. > > One note: Not catching a NoSectionError here would explicitly force [manifest] > to be present in the metadata.*, even if it's empty. > > I might check for presence first, what do you think? Done. https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser... chainedconfigparser.py:231: return base On 2018/04/19 11:12:41, Sebastian Noack wrote: > On 2018/04/19 11:08:14, tlucas wrote: > > On 2018/04/19 10:50:57, Sebastian Noack wrote: > > > Nit: Returning the dictionary seems redundant. It's the same object passed > in > > by > > > the calling code, hence the calling code already has it. You merely can > chain > > > the function call in a single line, but therefore you have one more line > here. > > > On the other hand, if this function doesn't return it there is no potential > > > confusion about the object being modified in-place. However, then we might > > want > > > to rename this function to add_section_to_dict() or serialize_section(). > > > > I wouldn't mind either solution - but we also might want to rename the method, > > when we're not returning anything anymore into e.g. merge_section_into(). > > > > What's your favorite? > > That is exactly what I said. My suggestions above were add_section_to_dict() and > serialize_section(); merge_section_into() is good to. Done.
LGTM
Message was sent while issue was closed.
I have a comment regarding this but it seems I'm too late. Well, it's cool then, it's only method naming. https://codereview.adblockplus.org/29743581/diff/29756606/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756606/chainedconfigparser... chainedconfigparser.py:160: def serialize_section_if_present(self, section, base): What this method does seems more like parsing than serialization. Perhaps it should be called "parse_section_if_present".
Message was sent while issue was closed.
https://codereview.adblockplus.org/29743581/diff/29756606/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756606/chainedconfigparser... chainedconfigparser.py:160: def serialize_section_if_present(self, section, base): On 2018/04/19 14:40:10, Vasily Kuznetsov wrote: > What this method does seems more like parsing than serialization. Perhaps it > should be called "parse_section_if_present". In the end it gets serialized (as JSON). But you might be right that what we do here is parsing it from the config. I guess, its too late now. Though, if anybody feeling strong about this and submits a Noissue patch, I would take it. 
|
