Issue 4047 - Improved configuration of converted JS files

Issue 4047 - Improved configuration of converted JS files

[Vasily Kuznetsov, 2016-08-29 16:56:40]

LAGTM. Just one suggestion to improve error reporting (see below).

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py#n...
packagerChrome.py:155: output_files[filename][2].append('{}={}'.format(arg,
item[1]))
Perhaps we should issue a more user-friendly message here if the options come
before the destination. It seems right now it will just `KeyError`.

[Sebastian Noack, 2016-08-30 12:40:31]

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py#n...
packagerChrome.py:155: output_files[filename][2].append('{}={}'.format(arg,
item[1]))
On 2016/08/29 16:56:40, Vasily Kuznetsov wrote:
> Perhaps we should issue a more user-friendly message here if the options come
> before the destination. It seems right now it will just `KeyError`.

Well, it's simpler to avoid that error than handling it. Did that.

[Vasily Kuznetsov, 2016-08-30 13:30:18]

LGTM.

Feel free to implement the suggestion about `defaultdict` below if you agree
with it.

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py#n...
packagerChrome.py:155: output_files[filename][2].append('{}={}'.format(arg,
item[1]))
On 2016/08/30 12:40:31, Sebastian Noack wrote:
> On 2016/08/29 16:56:40, Vasily Kuznetsov wrote:
> > Perhaps we should issue a more user-friendly message here if the options
come
> > before the destination. It seems right now it will just `KeyError`.
> 
> Well, it's simpler to avoid that error than handling it. Did that.

Indeed. Actually you can make the code even cleaner by using
`collections.defaultdict` for `args`.

[Sebastian Noack, 2016-08-30 14:06:50]

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350284/packagerChrome.py#n...
packagerChrome.py:155: output_files[filename][2].append('{}={}'.format(arg,
item[1]))
On 2016/08/30 13:30:18, Vasily Kuznetsov wrote:
> On 2016/08/30 12:40:31, Sebastian Noack wrote:
> > On 2016/08/29 16:56:40, Vasily Kuznetsov wrote:
> > > Perhaps we should issue a more user-friendly message here if the options
> come
> > > before the destination. It seems right now it will just `KeyError`.
> > 
> > Well, it's simpler to avoid that error than handling it. Did that.
> 
> Indeed. Actually you can make the code even cleaner by using
> `collections.defaultdict` for `args`.

Yeah, that is even better. Done.

[Vasily Kuznetsov, 2016-08-30 14:13:52]

https://codereview.adblockplus.org/29350281/diff/29350347/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350347/packagerChrome.py#n...
packagerChrome.py:150: args = collections.defaultdict(lambda: [])
This could be `collections.defaultdict(list)`.

[Sebastian Noack, 2016-08-30 14:23:06]

https://codereview.adblockplus.org/29350281/diff/29350347/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29350281/diff/29350347/packagerChrome.py#n...
packagerChrome.py:150: args = collections.defaultdict(lambda: [])
On 2016/08/30 14:13:52, Vasily Kuznetsov wrote:
> This could be `collections.defaultdict(list)`.

Done.

[Vasily Kuznetsov, 2016-08-30 14:32:33]

LGTM