Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29540555: Issue 5649 - Include full version number in AppxManifest.xml (Closed)

Created:
Sept. 11, 2017, 1:46 a.m. by Sebastian Noack
Modified:
Sept. 13, 2017, 3:53 p.m.
Visibility:
Public.

Description

Issue 5649 - Include full version number in AppxManifest.xml

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Re-add blank lines #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -4 lines) Patch
M packagerEdge.py View 1 1 chunk +0 lines, -1 line 0 comments Download
M templates/edge/AppxManifest.xml.tmpl View 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 7
Sebastian Noack
Sept. 11, 2017, 1:52 a.m. (2017-09-11 01:52:10 UTC) #1
tlucas
LGTM
Sept. 11, 2017, 8:23 a.m. (2017-09-11 08:23:09 UTC) #2
Vasily Kuznetsov
Hi Sebastian, It seems to me that it might be a better approach to remove ...
Sept. 11, 2017, 9:49 a.m. (2017-09-11 09:49:55 UTC) #3
Oleksandr
LGTM
Sept. 11, 2017, 9:51 a.m. (2017-09-11 09:51:39 UTC) #4
Sebastian Noack
On 2017/09/11 09:49:55, Vasily Kuznetsov wrote: > It seems to me that it might be ...
Sept. 11, 2017, 3:43 p.m. (2017-09-11 15:43:11 UTC) #5
Oleksandr
On 2017/09/11 15:43:11, Sebastian Noack wrote: > On 2017/09/11 09:49:55, Vasily Kuznetsov wrote: > > ...
Sept. 11, 2017, 4:12 p.m. (2017-09-11 16:12:21 UTC) #6
Vasily Kuznetsov
Sept. 13, 2017, 3:53 p.m. (2017-09-13 15:53:35 UTC) #7
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld