Issue 5649 - Include full version number in AppxManifest.xml

Issue 5649 - Include full version number in AppxManifest.xml

[tlucas, 2017-09-11 08:23:09]

LGTM

[Vasily Kuznetsov, 2017-09-11 09:49:55]

Hi Sebastian,

It seems to me that it might be a better approach to remove the version from the
result of `metadata.items('general')` before writing them into the `params`. Or
perhaps only copy the parameters that we need in the template instead of
everything. The template would look cleaner and simpler this way. If you don't
think this would be better, your approach is also fine.

Otherwise I just have one stylistic nit (see below), otherwise it looks good.
I'll be on vacation this week, so don't wait for my LGTM is everyone else is
happy.

Cheers,
Vasily

https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py#old...
packagerEdge.py:72: 
Removing these empty lines seems unrelated and it's also in violation of PEP8
(see https://www.python.org/dev/peps/pep-0008/#blank-lines).

[Oleksandr, 2017-09-11 09:51:39]

LGTM

[Sebastian Noack, 2017-09-11 15:43:11]

On 2017/09/11 09:49:55, Vasily Kuznetsov wrote:
> It seems to me that it might be a better approach to remove the version from
the
> result of `metadata.items('general')` before writing them into the `params`.
Or
> perhaps only copy the parameters that we need in the template instead of
> everything. The template would look cleaner and simpler this way. If you don't
> think this would be better, your approach is also fine.

The main reason I went for accessing the "metadata" object directly from the
template is because this is consistent with how manifest.json is generated. The
alternative you suggest wouldn't simplify anything, but merely move the logic
from the template into the controller. Furthermore, in particular if we
generically copy all options except "version" into the template context it is
less transparent/obvious where these data come from. The other way around, if we
explicitly copy the options we currently need, we would have to change the
controller as well when using more data in the template in the future.

https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py
File packagerEdge.py (left):

https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py#old...
packagerEdge.py:72: 
On 2017/09/11 09:49:54, Vasily Kuznetsov wrote:
> Removing these empty lines seems unrelated and it's also in violation of PEP8
> (see https://www.python.org/dev/peps/pep-0008/#blank-lines).

Sorry, this change was not intended. I added those blank lines again.

[Oleksandr, 2017-09-11 16:12:21]

On 2017/09/11 15:43:11, Sebastian Noack wrote:
> On 2017/09/11 09:49:55, Vasily Kuznetsov wrote:
> > It seems to me that it might be a better approach to remove the version from
> the
> > result of `metadata.items('general')` before writing them into the `params`.
> Or
> > perhaps only copy the parameters that we need in the template instead of
> > everything. The template would look cleaner and simpler this way. If you
don't
> > think this would be better, your approach is also fine.
> 
> The main reason I went for accessing the "metadata" object directly from the
> template is because this is consistent with how manifest.json is generated.
The
> alternative you suggest wouldn't simplify anything, but merely move the logic
> from the template into the controller. Furthermore, in particular if we
> generically copy all options except "version" into the template context it is
> less transparent/obvious where these data come from. The other way around, if
we
> explicitly copy the options we currently need, we would have to change the
> controller as well when using more data in the template in the future.
> 
> https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py
> File packagerEdge.py (left):
> 
>
https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py#old...
> packagerEdge.py:72: 
> On 2017/09/11 09:49:54, Vasily Kuznetsov wrote:
> > Removing these empty lines seems unrelated and it's also in violation of
PEP8
> > (see https://www.python.org/dev/peps/pep-0008/#blank-lines).
> 
> Sorry, this change was not intended. I added those blank lines again.

LGTM

[Vasily Kuznetsov, 2017-09-13 15:53:35]

On 2017/09/11 15:43:11, Sebastian Noack wrote:
> On 2017/09/11 09:49:55, Vasily Kuznetsov wrote:
> > It seems to me that it might be a better approach to remove the version from
> the
> > result of `metadata.items('general')` before writing them into the `params`.
> Or
> > perhaps only copy the parameters that we need in the template instead of
> > everything. The template would look cleaner and simpler this way. If you
don't
> > think this would be better, your approach is also fine.
> 
> The main reason I went for accessing the "metadata" object directly from the
> template is because this is consistent with how manifest.json is generated.
The
> alternative you suggest wouldn't simplify anything, but merely move the logic
> from the template into the controller. Furthermore, in particular if we
> generically copy all options except "version" into the template context it is
> less transparent/obvious where these data come from. The other way around, if
we
> explicitly copy the options we currently need, we would have to change the
> controller as well when using more data in the template in the future.

Yeah, I guess it makes sense.

> https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py
> File packagerEdge.py (left):
> 
>
https://codereview.adblockplus.org/29540555/diff/29540559/packagerEdge.py#old...
> packagerEdge.py:72: 
> On 2017/09/11 09:49:54, Vasily Kuznetsov wrote:
> > Removing these empty lines seems unrelated and it's also in violation of
PEP8
> > (see https://www.python.org/dev/peps/pep-0008/#blank-lines).
> 
> Sorry, this change was not intended. I added those blank lines again.

Cool! LGTM