Issue 29538644: Issue 5642 - Fix Windows Store devbuild upload routine (Closed)
|
Created:
Sept. 7, 2017, 11:02 p.m. by Oleksandr Modified:
Sept. 9, 2017, 10:57 p.m. CC:
kzar Visibility:
Public. |
DescriptionIssue 5642 - Fix Windows Store devbuild upload routine
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove redundant code and simplify formatting #
Total comments: 2
Patch Set 3 : Cosmetic fix #
Total comments: 1
Patch Set 4 : Fix indentation #MessagesTotal messages: 10
https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:620: NOTE: This code was part of the original review, but then it turned out submission worked without this explicit update. It is not clear what really happened, but now this is required again. See thread in original review: https://codereview.adblockplus.org/29374637/diff/29375717/sitescripts/extensi...
https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:625: {'fileStatus': 'PendingUpload'}) Please put closing parenthesis on a new line that starts in the same column as the line with opening parenthesis, for better readability: submission['applicationPackages'].append( {'fileStatus': 'PendingUpload'} ) https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:628: old_min_sys_ram = old_submission['minimumSystemRam'] It seems to be possible to avoid the temporary variables, without exceeding 79 chars per line: added_submission.update( minimumSystemRam=old_submission['minimumSystemRam'], minimumDirectXVersion=old_submission['minimumDirectXVersion'] ) I assume you made sure this is necessary in the first place, and just setting fileStatus for the old and new submission isn't sufficient? (It seems a little weird to me.)
https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29538645/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:628: old_min_sys_ram = old_submission['minimumSystemRam'] On 2017/09/08 00:15:55, Sebastian Noack wrote: > It seems to be possible to avoid the temporary variables, without exceeding 79 > chars per line: > > added_submission.update( > minimumSystemRam=old_submission['minimumSystemRam'], > minimumDirectXVersion=old_submission['minimumDirectXVersion'] > ) > > I assume you made sure this is necessary in the first place, and just setting > fileStatus for the old and new submission isn't sufficient? (It seems a little > weird to me.) I was pretty sure I have tested minimal set of changes required, but now that I retest only filename and status are required. So I am removing the redundant code and simplifying formatting even further.
Hey Oleksandr, as agreed with Vasily, i'm trying to take some load off him. Only one nit though, please see below :) Best, Tristan https://codereview.adblockplus.org/29538644/diff/29539697/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29539697/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:626: 'fileName': os.path.basename(self.path) nit: Please add a comma to the last line of a multi line definition, that way only one line has to be edited when adding additional items.
https://codereview.adblockplus.org/29538644/diff/29539697/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29539697/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:624: { FWIW, it would be OK to put the parenthesis an braces on one line like this: submission['applicationPackages'].append({ ... })
https://codereview.adblockplus.org/29538644/diff/29539846/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29538644/diff/29539846/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:624: 'fileStatus': 'PendingUpload', Nit: It should be just 4, not 8 spaces, per indentation level.
LGTM
LGTM 
|
