Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247923002:
Refactoring: Extract a method _copy_resources_to_wpt in deps_updater.py. (Closed)
|
Created:
4 years, 4 months ago by qyearsley Modified:
4 years, 4 months ago Reviewers:
Dirk Pranke CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactoring: Extract a method _copy_resources_to_wpt in deps_updater.py.
The main advantage of this is that it makes the main method shorter and more abstract, and provides a convenient place to put a comment about why we're copying over files from outside the imported repo into the new imported directory before committing.
Committed: https://crrev.com/5e3d57442de99b8ccba4dea4a14c3210213af6c8
Cr-Commit-Position: refs/heads/master@{#412588}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased #Patch Set 3 : Edited wording #Patch Set 4 : Reword #
Total comments: 1
Patch Set 5 : Rebased #Patch Set 6 : Edited comment to remove unnecessary words. #Messages
Total messages: 22 (13 generated)
Description was changed from ========== Refactoring: Extract a method _copy_resources_to_wpt in deps_updater.py. ========== to ========== Refactoring: Extract a method _copy_resources_to_wpt in deps_updater.py. The main advantage of this is that it makes the main method shorter and more abstract, and provides a convenient place to put a comment about why we're copying over files from outside the imported repo into the new imported directory before committing. ==========
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2247923002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2247923002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:127: This is necessary because we want to be used when running imported tests Nit: what is it that we want to be used? This wording is awkward.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/15 at 23:40:14, dpranke wrote: > lgtm > > https://codereview.chromium.org/2247923002/diff/1/third_party/WebKit/Tools/Sc... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): > > https://codereview.chromium.org/2247923002/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:127: This is necessary because we want to be used when running imported tests > Nit: what is it that we want to be used? This wording is awkward. Yep, that wording was badly mangled. What I meant to say was, I think that we're copying over these resources because we have our own versions which we want to use when running web-platform-tests. I've now replaced that paragraph with: There are some resource files in our repository that we want to replace imported files from the upstream repository. Seem OK?
On 2016/08/16 22:25:30, qyearsley wrote: > I've now replaced that paragraph with: > > There are some resource files in our repository that we want to replace > imported files from the upstream repository. I would say "want to use instead of the upstream versions" rather than "want to replace imported files from the upstream repository". Or something like that.
On 2016/08/16 at 22:27:06, dpranke wrote: > On 2016/08/16 22:25:30, qyearsley wrote: > > I've now replaced that paragraph with: > > > > There are some resource files in our repository that we want to replace > > imported files from the upstream repository. > > I would say "want to use instead of the upstream versions" rather than "want to replace imported files from the upstream repository". Or something like that. Yep, sounds better to me; updated.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2247923002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2247923002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:131: There are some resource files in our repository that we want to use One last nit: s/There are some resource/These are/ and s/want to use/use/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2247923002/#ps100001 (title: "Edited comment to remove unnecessary words.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Extract a method _copy_resources_to_wpt in deps_updater.py. The main advantage of this is that it makes the main method shorter and more abstract, and provides a convenient place to put a comment about why we're copying over files from outside the imported repo into the new imported directory before committing. ========== to ========== Refactoring: Extract a method _copy_resources_to_wpt in deps_updater.py. The main advantage of this is that it makes the main method shorter and more abstract, and provides a convenient place to put a comment about why we're copying over files from outside the imported repo into the new imported directory before committing. Committed: https://crrev.com/5e3d57442de99b8ccba4dea4a14c3210213af6c8 Cr-Commit-Position: refs/heads/master@{#412588} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5e3d57442de99b8ccba4dea4a14c3210213af6c8 Cr-Commit-Position: refs/heads/master@{#412588} |
