Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(611)

Issue 29716693: Issue 6371 - Update buildtools dep. to c830dfa08e2f, use AMO-signing API (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by tlucas
Modified:
6 months ago
CC:
mathias, kzar
Base URL:
https://hg.adblockplus.org/abpssembly/file/1e38c3375fa3
Visibility:
Public.

Description

Issue 6371 - Update buildtools dep. to c830dfa08e2f, use AMO-signing API * Refactored some reused logic * Implemented full usage of the AMO signing api * Introduced a lockfile-mechanism for extensions that have to be downloaded * Introduce a "--download" parameter, causing createNightlies.py to only download files (this is to be called by a new cronjob) ** IMPORTANT ** This and https://codereview.adblockplus.org/29716699 have to land simultaneously.

Patch Set 1 #

Total comments: 25

Patch Set 2 : #

Total comments: 11

Patch Set 3 : #

Total comments: 3

Patch Set 4 : #

Patch Set 5 : NO CHANGE - Rebase against current master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -115 lines) Patch
M dependencies View 1 chunk +1 line, -1 line 0 comments Download
M ensure_dependencies.py View 3 chunks +12 lines, -3 lines 0 comments Download
M sitescripts/extensions/bin/createNightlies.py View 1 2 3 20 chunks +210 lines, -46 lines 0 comments Download
A sitescripts/extensions/template/gecko.json View 1 2 1 chunk +16 lines, -0 lines 0 comments Download
M tox.ini View 1 2 3 4 2 chunks +65 lines, -65 lines 0 comments Download

Messages

Total messages: 12
tlucas
Patch Set 1 https://codereview.adblockplus.org/29716693/diff/29716694/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/ensure_dependencies.py#newcode1 ensure_dependencies.py:1: #!/usr/bin/env python NOTE: All changes in ...
6 months, 2 weeks ago (2018-03-07 22:14:19 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py#newcode422 sitescripts/extensions/bin/createNightlies.py:422: def open_downloads_loackfile(self): Typo: loackfile => lockfile (or lock_file) https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py#newcode422 ...
6 months, 2 weeks ago (2018-03-08 02:39:23 UTC) #2
tlucas
Patch Set 2 * Addressed Sebastian's comments * Made method-naming consistent https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): ...
6 months, 2 weeks ago (2018-03-08 09:23:05 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py#newcode424 sitescripts/extensions/bin/createNightlies.py:424: current = {} On 2018/03/08 09:23:05, tlucas wrote: > ...
6 months, 2 weeks ago (2018-03-09 00:33:19 UTC) #4
tlucas
Patch Set 3 Addressed Sebastian's comments (except .format(), see below). https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensions/bin/createNightlies.py#newcode424 ...
6 months, 2 weeks ago (2018-03-09 08:12:58 UTC) #5
Sebastian Noack
Looks almost good to me. Just one nit, I missed before. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): ...
6 months, 2 weeks ago (2018-03-09 16:16:28 UTC) #6
tlucas
Patch Set 4 * Addressed Sebastian's nit https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensions/bin/createNightlies.py File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensions/bin/createNightlies.py#newcode515 sitescripts/extensions/bin/createNightlies.py:515: (('Content-Type', content_type),), ...
6 months, 1 week ago (2018-03-12 07:22:46 UTC) #7
Sebastian Noack
LGTM
6 months, 1 week ago (2018-03-12 16:25:10 UTC) #8
Vasily Kuznetsov
I don't have very much of substance to add, but there's one formatting-related point (see ...
6 months, 1 week ago (2018-03-13 17:50:00 UTC) #9
Sebastian Noack
On 2018/03/13 17:50:00, Vasily Kuznetsov wrote: > Sebastian, what is your reason for preferring to ...
6 months, 1 week ago (2018-03-13 18:01:30 UTC) #10
Vasily Kuznetsov
On 2018/03/13 18:01:30, Sebastian Noack wrote: > On 2018/03/13 17:50:00, Vasily Kuznetsov wrote: > > ...
6 months, 1 week ago (2018-03-13 21:55:44 UTC) #11
Sebastian Noack
6 months, 1 week ago (2018-03-13 23:32:48 UTC) #12
On 2018/03/13 21:55:44, Vasily Kuznetsov wrote:
> On 2018/03/13 18:01:30, Sebastian Noack wrote:
> > On 2018/03/13 17:50:00, Vasily Kuznetsov wrote:
> > > Sebastian, what is your reason for preferring to have no comma after the
> last
> > > argument? How about list and dict literals that are similarly formatted?
> > 
> > Mostly for consistence. I don't recall haven't seen (m)any code in our code
> base
> > (or in the wild), where a trailing comma was added after the last argument
in
> a
> > function call. Lists and dictionaries are different, however. Those are
> variable
> > data structures of uniform items. Adding/Removing items in a list/dict is
> > common, and the trailing comma keeps those changes minimal. The arguments
> passed
> > to a function, however, are structured. Adding/Removing arguments requires
> > changing the way in which they are processed.
> 
> Indeed arguments of functions change less often, but sometimes they do change.
> Functions that take variable number of arguments or keyword arguments are
rather
> common in Python and in those cases adding or removing arguments is not all
> uncommon.

Functions that take variable number of arguments and process them in a uniform
way (as opposed to passing them through to another function which expects the
arguments to be in a particular structure) exist but are rather rare. Usually,
it is a better practice to just take a sequence as a single argument (one reason
being that it doesn't force the input being converted to a tuple, causing
unnecessary overhead in particular when you have a lazily evaluated sequence).

> It's also simpler to just have the same rule for wrapping and indentation of
> lists, dicts, sets, tuples and function calls. Syntactically they all look
> pretty similar and it's easier to just remember one simple rule.

I mostly want to avoid forcing some esoteric notation on our coding style, just
because it makes a linter rule slightly simpler to implement. Feel free to show
me examples (either from our code base or other popular projects written in
Python) where argument lists have trailing commas.

> In any case, this review seems like a rather bad place to discuss this so
don't
> let me hold you from landing it -- this one instance doesn't matter that much.
> We can continue in the review for the coding style change that I'm planning to
> create soon.

Yes, this is definitely the wrong place for this discussion. Let's finally land
this change and create a separate issue if you want to keep discussing this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5