Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960673003:
[cronet] replace filter with comprehension in build script (Closed)
|
Created:
3 years, 5 months ago by lilyhoughton Modified:
3 years, 5 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cronet] replace filter with comprehension in build script
BUG=711409
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2960673003
Cr-Commit-Position: refs/heads/master@{#483001}
Committed: https://chromium.googlesource.com/chromium/src/+/f1a5a6b25d91ad86812bdc7a214d5d71d9c61de5
Patch Set 1 #
Total comments: 1
Patch Set 2 : make more readable #
Total comments: 2
Patch Set 3 : replace generator with list comprehension; rename appropriately #Messages
Total messages: 15 (7 generated)
Description was changed from ========== [cronet] replace filter with comprehension in build script BUG=711409 ========== to ========== [cronet] replace filter with comprehension in build script BUG=711409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== [cronet] replace filter with comprehension in build script BUG=711409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [cronet] replace filter with comprehension in build script BUG=711409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
lilyhoughton@chromium.org changed reviewers: + xunjieli@chromium.org
Note that we still have an error E: 58,16: Module 'chrome_telemetry_build.chromium_config' has no 'GetTelemetryDir' member (no-member) though I think this is unrelated.
https://codereview.chromium.org/2960673003/diff/1/components/cronet/tools/gen... File components/cronet/tools/generate_accept_languages.py (right): https://codereview.chromium.org/2960673003/diff/1/components/cronet/tools/gen... components/cronet/tools/generate_accept_languages.py:31: if accept_langs) This is getting rather long. Can you separate it out and make it more readable?
https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... File components/cronet/tools/generate_accept_languages.py (right): https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... components/cronet/tools/generate_accept_languages.py:28: accept_langs_dict = (extract_accept_langs(filename) On 2017/06/27 17:51:45, xunjieli wrote: > https://codereview.chromium.org/2960673003/diff/1/components/cronet/tools/gen... > File components/cronet/tools/generate_accept_languages.py (right): > > https://codereview.chromium.org/2960673003/diff/1/components/cronet/tools/gen... > components/cronet/tools/generate_accept_languages.py:31: if accept_langs) > This is getting rather long. Can you separate it out and make it more readable? How is this?
lgtm mod one comment below. https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... File components/cronet/tools/generate_accept_languages.py (right): https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... components/cronet/tools/generate_accept_languages.py:28: accept_langs_dict = (extract_accept_langs(filename) Shouldn't Python list comprehension use square brackets [] rather than parentheses? Please also rename |accept_langs_dict| to |accept_langs_list|
On 2017/06/28 14:53:53, xunjieli wrote: > lgtm mod one comment below. > > https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... > File components/cronet/tools/generate_accept_languages.py (right): > > https://codereview.chromium.org/2960673003/diff/20001/components/cronet/tools... > components/cronet/tools/generate_accept_languages.py:28: accept_langs_dict = > (extract_accept_langs(filename) > Shouldn't Python list comprehension use square brackets [] rather than > parentheses? > > Please also rename |accept_langs_dict| to |accept_langs_list| It's a generator comprehension instead of a true list comprehension so that we only do the iteration once. Probably the (small, unverified) performance benefit isn't worth the loss in clarity though, so Done.
The CQ bit was checked by lilyhoughton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2960673003/#ps40001 (title: "replace generator with list comprehension; rename appropriately")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> It's a generator comprehension instead of a true list comprehension so that we > only do the iteration once. Probably the (small, unverified) performance > benefit isn't worth the loss in clarity though, so Done. I see. Good to know!
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498663498361940,
"parent_rev": "84a463f08a43176366b7643ed485d42894b61006", "commit_rev":
"f1a5a6b25d91ad86812bdc7a214d5d71d9c61de5"}
Message was sent while issue was closed.
Description was changed from ========== [cronet] replace filter with comprehension in build script BUG=711409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [cronet] replace filter with comprehension in build script BUG=711409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2960673003 Cr-Commit-Position: refs/heads/master@{#483001} Committed: https://chromium.googlesource.com/chromium/src/+/f1a5a6b25d91ad86812bdc7a214d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f1a5a6b25d91ad86812bdc7a214d... |
