Issue 4719 - Edge packager does not generate an info module

Issue 4719 - Edge packager does not generate an info module

[Vasily Kuznetsov, 2016-12-09 11:46:23]

Please note that this change breaks the tests (more specifically the code style
check):

$ tox

.... some output skipped ......

py27 runtests: commands[1] | flake8
./packagerEdge.py:175:80: E501 line too long (81 > 79 characters)
./packagerEdge.py:176:9: E129 visually indented line with same indent as next
logical line

[kzar, 2016-12-12 12:14:09]

Once style issues Vasily pointed out are fixed LGTM

[kzar, 2016-12-13 09:19:22]

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py#new...
packagerEdge.py:175: 'lib/info.js' in metadata.get('general',
'backgroundScripts').split()
IMO this looks kind of ugly, I don't like how the second clause does not end
with the "and". I'll leave it up to Vasily and Sebastian to decide anyway, (FWIW
I checked and Tox does pass.)

[Vasily Kuznetsov, 2016-12-13 18:49:59]

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py#new...
packagerEdge.py:175: 'lib/info.js' in metadata.get('general',
'backgroundScripts').split()
On 2016/12/13 09:19:22, kzar wrote:
> IMO this looks kind of ugly, I don't like how the second clause does not end
> with the "and". I'll leave it up to Vasily and Sebastian to decide anyway,
(FWIW
> I checked and Tox does pass.)

I would have computed the background scripts list beforehand to make the
condition shorter and more readable. Maybe something like this:

if metadata.has_option('general', 'backgroundScripts'):
    bg_scripts = metadata.get('general', 'backgroundScripts').split()
    if 'lib/info.js' in bg_scripts and 'lib/info.js' not in files:
        # yada yada yada

Not a big deal though, LGTM from me anyway.

[Sebastian Noack, 2016-12-15 13:58:06]

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py#new...
packagerEdge.py:175: 'lib/info.js' in metadata.get('general',
'backgroundScripts').split()
On 2016/12/13 18:49:58, Vasily Kuznetsov wrote:
> On 2016/12/13 09:19:22, kzar wrote:
> > IMO this looks kind of ugly, I don't like how the second clause does not end
> > with the "and". I'll leave it up to Vasily and Sebastian to decide anyway,
> (FWIW
> > I checked and Tox does pass.)
> 
> I would have computed the background scripts list beforehand to make the
> condition shorter and more readable. Maybe something like this:
> 
> if metadata.has_option('general', 'backgroundScripts'):
>     bg_scripts = metadata.get('general', 'backgroundScripts').split()
>     if 'lib/info.js' in bg_scripts and 'lib/info.js' not in files:
>         # yada yada yada
> 
> Not a big deal though, LGTM from me anyway.

I agree with Vasily here. Also, is it even necessery to check for |'lib/info.js'
in files|? On the other hand, the code is consistent with what we have in
packagerChrome.py. So if we change it here we should also change it their for
consistency. No strong opinion from my side.

Also, we probably should adapt the tests to cover this code path and make sure
that the file got added.

[Oleksandr, 2016-12-22 02:53:24]

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py
File packagerEdge.py (right):

https://codereview.adblockplus.org/29367145/diff/29367277/packagerEdge.py#new...
packagerEdge.py:175: 'lib/info.js' in metadata.get('general',
'backgroundScripts').split()
On 2016/12/15 13:58:06, Sebastian Noack wrote:
> I agree with Vasily here. Also, is it even necessery to check for
|'lib/info.js'
> in files|? On the other hand, the code is consistent with what we have in
> packagerChrome.py. So if we change it here we should also change it their for
> consistency. No strong opinion from my side.

I think touching packagerChrome.py would require a separate issue. Omitting that
here.

> Also, we probably should adapt the tests to cover this code path and make sure
> that the file got added.

Done.

[Sebastian Noack, 2016-12-22 15:24:12]

https://codereview.adblockplus.org/29367145/diff/29369496/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29367145/diff/29369496/tests/metadata.edge...
tests/metadata.edge:20: 
It seems you accidentally added a blank line here.

[Sebastian Noack, 2016-12-22 16:30:49]

LGTM