Issue 29330140: Issue 3312 - Serve release update manifests for Adblock Browser (Closed)
|
Created:
Nov. 13, 2015, 7:41 a.m. by Felix Dahlke Modified:
Nov. 19, 2015, 3:55 p.m. Reviewers:
Sebastian Noack CC:
Wladimir Palant, mathias Visibility:
Public. |
DescriptionIssue 3312 - Serve release update manifests for Adblock Browser
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use os.path.splitext, remove devbuild flag #
Total comments: 2
Patch Set 3 : Don't call get_config() multiple times in a row #
Total comments: 5
Patch Set 4 : Add some whitespace #
MessagesTotal messages: 10
https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:52: apk_path = re.sub(r"\.json$", ".apk", json_path) Checking for the file extension here is redundant. How about using |os.path.splitext(json_path)[0] + ".apk"|? https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:64: builds_url = "%s/adblockbrowser" % nightlies_url.rstrip("/") rstrip("/") is duplicated. How about moving it below the if/else? https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:110: return _handle_request(environ, start_response, False) I think the code flow would be simpler when you determine the paths here and pass them to _handle_request rather than switching the logic inside _handle_request based on a flag.
New patch set is up. https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:52: apk_path = re.sub(r"\.json$", ".apk", json_path) On 2015/11/13 22:07:39, Sebastian Noack wrote: > Checking for the file extension here is redundant. How about using > |os.path.splitext(json_path)[0] + ".apk"|? Done. https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:64: builds_url = "%s/adblockbrowser" % nightlies_url.rstrip("/") On 2015/11/13 22:07:39, Sebastian Noack wrote: > rstrip("/") is duplicated. How about moving it below the if/else? How would that look? Note that I need to call rstrip on the URL I get from get_config().get(), only for release builds that's the same as builds_url. https://codereview.adblockplus.org/29330140/diff/29330141/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:110: return _handle_request(environ, start_response, False) On 2015/11/13 22:07:39, Sebastian Noack wrote: > I think the code flow would be simpler when you determine the paths here and > pass them to _handle_request rather than switching the logic inside > _handle_request based on a flag. Done.
https://codereview.adblockplus.org/29330140/diff/29330209/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330209/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_url = get_config().get("extensions", "downloadsURL").rstrip("/") How about caching the config object instead calling get_config twice?
New patch set is up. https://codereview.adblockplus.org/29330140/diff/29330209/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330209/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_url = get_config().get("extensions", "downloadsURL").rstrip("/") On 2015/11/14 21:09:23, Sebastian Noack wrote: > How about caching the config object instead calling get_config twice? Done.
https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_dir = config.get("extensions", "downloadsDirectory") Mit: Please set newlines consistent with the handler below.
https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_dir = config.get("extensions", "downloadsDirectory") On 2015/11/19 09:48:40, Sebastian Noack wrote: > Mit: Please set newlines consistent with the handler below. IMHO, readlines only help readability in rare cases, and I only use them when I believe that's the case. Up here, in a four line function doing just one thing, I don't think it helps. If you do disagree and think it helps readability here though, I'll change it.
https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_dir = config.get("extensions", "downloadsDirectory") On 2015/11/19 09:50:34, Felix Dahlke wrote: > On 2015/11/19 09:48:40, Sebastian Noack wrote: > > Mit: Please set newlines consistent with the handler below. > > IMHO, readlines only help readability in rare cases, and I only use them when I > believe that's the case. Up here, in a four line function doing just one thing, > I don't think it helps. > > If you do disagree and think it helps readability here though, I'll change it. s/readlines/newlines/ :)
https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_dir = config.get("extensions", "downloadsDirectory") On 2015/11/19 09:50:34, Felix Dahlke wrote: > On 2015/11/19 09:48:40, Sebastian Noack wrote: > > Mit: Please set newlines consistent with the handler below. > > IMHO, readlines only help readability in rare cases, and I only use them when I > believe that's the case. Up here, in a four line function doing just one thing, > I don't think it helps. > > If you do disagree and think it helps readability here though, I'll change it. I think it does, but I guess it's matter of taste. LGTM
New patch set up with more whitespace. Being such a minor change, I'll consider this approved and land it. https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... File sitescripts/extensions/web/adblockbrowserUpdates.py (right): https://codereview.adblockplus.org/29330140/diff/29330408/sitescripts/extensi... sitescripts/extensions/web/adblockbrowserUpdates.py:100: builds_dir = config.get("extensions", "downloadsDirectory") On 2015/11/19 09:53:01, Sebastian Noack wrote: > On 2015/11/19 09:50:34, Felix Dahlke wrote: > > On 2015/11/19 09:48:40, Sebastian Noack wrote: > > > Mit: Please set newlines consistent with the handler below. > > > > IMHO, readlines only help readability in rare cases, and I only use them when > I > > believe that's the case. Up here, in a four line function doing just one > thing, > > I don't think it helps. > > > > If you do disagree and think it helps readability here though, I'll change it. > > I think it does, but I guess it's matter of taste. LGTM Changed it since you do disagree. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
