Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(223)

Issue 29743581: Issue 6552 - Support arbitrary manifest values (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by tlucas
Modified:
5 months ago
CC:
kzar
Base URL:
https://hg.adblockplus.org/buildtools/file/a3db4a1a49e8
Visibility:
Public.

Description

Issue 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+496 lines, -347 lines) Patch
M chainedconfigparser.py View 1 2 3 4 5 6 7 8 2 chunks +69 lines, -3 lines 2 comments Download
M packagerChrome.py View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -11 lines 0 comments Download
M templates/manifest.json.tmpl View 1 2 3 4 5 2 chunks +0 lines, -26 lines 0 comments Download
M tests/expecteddata/manifest_chrome_development_build.json View 1 2 3 1 chunk +43 lines, -32 lines 0 comments Download
M tests/expecteddata/manifest_chrome_devenv.json View 1 2 3 1 chunk +43 lines, -32 lines 0 comments Download
M tests/expecteddata/manifest_chrome_release_build.json View 1 2 3 1 chunk +42 lines, -31 lines 0 comments Download
M tests/expecteddata/manifest_edge_development_build.json View 1 2 3 1 chunk +44 lines, -33 lines 0 comments Download
M tests/expecteddata/manifest_edge_devenv.json View 1 2 3 1 chunk +45 lines, -34 lines 0 comments Download
M tests/expecteddata/manifest_edge_release_build.json View 1 2 3 1 chunk +44 lines, -33 lines 0 comments Download
M tests/expecteddata/manifest_gecko_development_build.json View 1 2 3 1 chunk +44 lines, -33 lines 0 comments Download
M tests/expecteddata/manifest_gecko_devenv.json View 1 2 3 1 chunk +45 lines, -34 lines 0 comments Download
M tests/expecteddata/manifest_gecko_release_build.json View 1 2 3 1 chunk +44 lines, -33 lines 0 comments Download
M tests/metadata.chrome View 1 2 3 4 5 6 2 chunks +24 lines, -9 lines 0 comments Download
M tests/metadata.edge View 1 2 3 4 5 6 2 chunks +7 lines, -3 lines 0 comments Download

Messages

Total messages: 26
tlucas
Patch Set 1 * Support externally_connectable in manifest.json
5 months, 2 weeks ago (2018-04-05 10:23:07 UTC) #1
Sebastian Noack
Just an idea, but I wonder whether it begins to make sense to implement a ...
5 months, 2 weeks ago (2018-04-05 19:14:03 UTC) #2
tlucas
On 2018/04/05 19:14:03, Sebastian Noack wrote: > Just an idea, but I wonder whether it ...
5 months, 2 weeks ago (2018-04-05 19:24:44 UTC) #3
tlucas
Patch Set 2 * Implemented support for arbitrary values in a metadata's `manifest`-section * Modified ...
5 months, 1 week ago (2018-04-11 09:46:34 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py#newcode210 chainedconfigparser.py:210: return v * I cannot think of any relevant ...
5 months, 1 week ago (2018-04-12 12:19:24 UTC) #5
tlucas
Patch Set 3 * Use OrderedDict in parsing * reindented metadata.chrome * replied to Sebastian's ...
5 months, 1 week ago (2018-04-12 13:47:39 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29749559/chainedconfigparser.py#newcode210 chainedconfigparser.py:210: return v On 2018/04/12 13:47:39, tlucas wrote: > On ...
5 months, 1 week ago (2018-04-12 14:39:29 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.json.tmpl File templates/manifest.json.tmpl (right): https://codereview.adblockplus.org/29743581/diff/29750596/templates/manifest.json.tmpl#newcode120 templates/manifest.json.tmpl:120: {%- endif %} I just noticed, so we still ...
5 months, 1 week ago (2018-04-12 15:10:49 UTC) #8
tlucas
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.py#newcode210 ...
5 months, 1 week ago (2018-04-12 15:29:30 UTC) #9
Sebastian Noack
On 2018/04/12 15:29:30, tlucas wrote: > I wouldn't mind not distinguishing between float and int, ...
5 months, 1 week ago (2018-04-12 16:08:42 UTC) #10
tlucas
On 2018/04/12 16:08:42, Sebastian Noack wrote: > On 2018/04/12 15:29:30, tlucas wrote: > > I ...
5 months, 1 week ago (2018-04-12 16:11:12 UTC) #11
tlucas
Patch Set 4 * Made strictly typing json values less obscure * Removed explicit reference ...
5 months, 1 week ago (2018-04-13 09:14:59 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py#newcode161 chainedconfigparser.py:161: def as_json_object(self, section): Actually there isn't anything specific to ...
5 months, 1 week ago (2018-04-14 00:33:07 UTC) #13
tlucas
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py#newcode161 chainedconfigparser.py:161: def as_json_object(self, section): On 2018/04/14 00:33:07, Sebastian Noack wrote: ...
5 months, 1 week ago (2018-04-14 08:36:24 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/chainedconfigparser.py#newcode161 chainedconfigparser.py:161: def as_json_object(self, section): On 2018/04/14 08:36:23, tlucas wrote: > ...
5 months, 1 week ago (2018-04-14 08:54:27 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29743581/diff/29751570/packagerChrome.py#newcode140 packagerChrome.py:140: manifest = json.dumps(data, sort_keys=True, indent=2) If we inject the ...
5 months, 1 week ago (2018-04-14 09:08:40 UTC) #16
tlucas
Patch Set 6 * Use newlines as array definers * Removed redundant "OrderedDict" * Merging ...
5 months ago (2018-04-18 14:46:05 UTC) #17
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29755749/chainedconfigparser.py#newcode203 chainedconfigparser.py:203: return [parse_values(x) for x in v] Checking for lists ...
5 months ago (2018-04-18 15:59:01 UTC) #18
tlucas
Patch Set 7 * Removed redundant recursion * Properly deep merge into given dictionary * ...
5 months ago (2018-04-19 10:01:41 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py#newcode171 chainedconfigparser.py:171: will define the option's value as an array. Nit: ...
5 months ago (2018-04-19 10:50:59 UTC) #20
tlucas
Patch Set 8 * Addressed Sebastian's comments * Discussion about section absence handling, method name ...
5 months ago (2018-04-19 11:08:15 UTC) #21
Sebastian Noack
https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py File chainedconfigparser.py (right): https://codereview.adblockplus.org/29743581/diff/29756576/chainedconfigparser.py#newcode231 chainedconfigparser.py:231: return base On 2018/04/19 11:08:14, tlucas wrote: > On ...
5 months ago (2018-04-19 11:12:42 UTC) #22
tlucas
Patch Set 9 * Handle absence of the given section * Rename the new method ...
5 months ago (2018-04-19 11:45:15 UTC) #23
Sebastian Noack
LGTM
5 months ago (2018-04-19 11:47:26 UTC) #24
Vasily Kuznetsov
I have a comment regarding this but it seems I'm too late. Well, it's cool ...
5 months ago (2018-04-19 14:40:11 UTC) #25
Sebastian Noack
5 months ago (2018-04-19 15:03:00 UTC) #26
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5