Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576353003:
Change ChromiumCommit.format_patch to use a filtered list of files (Closed)
DescriptionChange ChromiumCommit.format_patch to use a filtered list of files
(2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed.
BUG=657117, 671890
Committed: https://crrev.com/4a865436e0b0437e17772c8b8828c77b55e7328e
Cr-Commit-Position: refs/heads/master@{#440301}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 8
Patch Set 3 : Address CL feedback #
Total comments: 7
Patch Set 4 : Address CL feedback #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Change ChromiumCommit.format_patch to use a filtered list of files BUG=657117,671890 ========== to ========== Change ChromiumCommit.format_patch to use a filtered list of files This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 ==========
jeffcarp@chromium.org changed reviewers: + qyearsley@chromium.org
Description was changed from ========== Change ChromiumCommit.format_patch to use a filtered list of files This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 ========== to ========== Change ChromiumCommit.format_patch to use a filtered list of files (2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 ==========
On 2016/12/16 at 00:46:52, jeffcarp wrote: > This is rebased and ready to be reviewed.
https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right): https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:68: """Makes a patch with just changes in files in the WPT for a given commit.""" "in the WPT" -> "in the WPT dir" https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:74: blacklist = ['MANIFEST.json', 'resources/testharnessreport.js'] Just in case backslashes might be used on Windows in some cases, this could also be: blacklist = [ 'MANIFEST.json', self.host.filesystem.join('resources', 'testharnessreport.js'), ] https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:75: qualified_blacklist = [CHROMIUM_WPT_DIR + f for f in blacklist] I'm wondering if there's a better word to use here besides "qualified"... https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:80: """ Nit: the closing quote can go on the previous line. https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:12: CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' (can be removed)
The CQ bit was checked by jeffcarp@chromium.org to run a CQ dry run
Addressed CL feedback. https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right): https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:75: qualified_blacklist = [CHROMIUM_WPT_DIR + f for f in blacklist] On 2016/12/19 at 17:50:08, qyearsley wrote: > I'm wondering if there's a better word to use here besides "qualified"... I can't think of anything more fitting but open to any suggestions. I was thinking of "fully qualified domain name" https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:12: CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' On 2016/12/19 at 17:50:08, qyearsley wrote: > (can be removed) Oops thanks, I think that was a rebase error on my end.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM to commit, with a few random optional comments :-) Another comment: For most interaction with git in most of webkitpy, everything is done through methods on webkitpy.common.checkout.git.Git. Since we're doing a lot of invocations of git that are only in this module, it's fine as is, but if there are any common git operations that we do in multiple places, they can also be extracted to that common git module. https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right): https://codereview.chromium.org/2576353003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:75: qualified_blacklist = [CHROMIUM_WPT_DIR + f for f in blacklist] On 2016/12/19 at 21:19:20, jeffcarp wrote: > On 2016/12/19 at 17:50:08, qyearsley wrote: > > I'm wondering if there's a better word to use here besides "qualified"... > > I can't think of anything more fitting but open to any suggestions. I was thinking of "fully qualified domain name" Yep, sometimes there's no clear proper name for something. In this case, at least, it's clear that what's meant is that it's a file path that's relative to the Chromium checkout root. One possible but longer name would be chromium_relative_blacklist, but it's fine as-is too. https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:90: self.sha, '--'] + filtered_files, cwd=self.absolute_chromium_dir()) [Bike-shedding about format/style] I wonder if it's any better to format this as: return self.host.executive.run_command([ 'git', 'format-patch', '-1', '--stdout', self.sha, '--' ] + filtered_files, cwd=self.absolute_chromium_dir()) Either way is fine. https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py:41: pos = 'Cr-Commit-Position: refs/heads/master@{#789}' Nit: if you want to avoid abbreviation variable names, this could be position_string. https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:12: CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' Optionally, no need to remove this line; this file could be removed from the CL.
Addressed feedback. Will create a bug to move to webkitpy.common.checkout.git.Git. https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:90: self.sha, '--'] + filtered_files, cwd=self.absolute_chromium_dir()) On 2016/12/21 at 00:24:46, qyearsley wrote: > [Bike-shedding about format/style] > > I wonder if it's any better to format this as: > > return self.host.executive.run_command([ > 'git', 'format-patch', '-1', '--stdout', self.sha, '--' > ] + filtered_files, cwd=self.absolute_chromium_dir()) > > Either way is fine. I'm still learning the intricacies of python formatting. That looks good (also it seems more javascripty). https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py:41: pos = 'Cr-Commit-Position: refs/heads/master@{#789}' On 2016/12/21 at 00:24:46, qyearsley wrote: > Nit: if you want to avoid abbreviation variable names, this could be position_string. Done, abbreviated variables begone! I chose to name it position_footer because putting the variable type in variables makes me feel kind of weird for reasons I can't accurately describe. https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:12: CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/' On 2016/12/21 at 00:24:46, qyearsley wrote: > Optionally, no need to remove this line; this file could be removed from the CL. Done
LGTM to commit :-) https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py (right): https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py:41: pos = 'Cr-Commit-Position: refs/heads/master@{#789}' On 2016/12/21 at 19:12:52, jeffcarp wrote: > On 2016/12/21 at 00:24:46, qyearsley wrote: > > Nit: if you want to avoid abbreviation variable names, this could be position_string. > > Done, abbreviated variables begone! I chose to name it position_footer because putting the variable type in variables makes me feel kind of weird for reasons I can't accurately describe. Yeah -- the main reason I've heard before is that in statically-typed languages, putting the type in the variable name is redundant, e.g. in C++ if you used `std::string position_string` you would just be repeating yourself. But even in Python, the type of the variable should be obvious, especially in this case. I think the reason why I didn't suggest "position", is that "commit position" has a very more specific meaning elsewhere: it has to be just the "refs/heads/master@{#789}" part, including the ref and number, and nothing else. position_footer is definitely the best variable name here.
The CQ bit was checked by jeffcarp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/21 at 19:19:56, qyearsley wrote: > LGTM to commit :-) > > https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py (right): > > https://codereview.chromium.org/2576353003/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit_unittest.py:41: pos = 'Cr-Commit-Position: refs/heads/master@{#789}' > On 2016/12/21 at 19:12:52, jeffcarp wrote: > > On 2016/12/21 at 00:24:46, qyearsley wrote: > > > Nit: if you want to avoid abbreviation variable names, this could be position_string. > > > > Done, abbreviated variables begone! I chose to name it position_footer because putting the variable type in variables makes me feel kind of weird for reasons I can't accurately describe. > > Yeah -- the main reason I've heard before is that in statically-typed languages, putting the type in the variable name is redundant, e.g. in C++ if you used `std::string position_string` you would just be repeating yourself. > > But even in Python, the type of the variable should be obvious, especially in this case. > > I think the reason why I didn't suggest "position", is that "commit position" has a very more specific meaning elsewhere: it has to be just the "refs/heads/master@{#789}" part, including the ref and number, and nothing else. > > position_footer is definitely the best variable name here. I thought I had sent this to the CQ this morning but apparently not ¯\_(ツ)_/¯
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482364642047610,
"parent_rev": "e9de811ebb9eb1dd9659a1de69d1b69e6aa9a36e", "commit_rev":
"ca715dd0aae5962ad78c261b2cff5a9b51eec683"}
Message was sent while issue was closed.
Description was changed from ========== Change ChromiumCommit.format_patch to use a filtered list of files (2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 ========== to ========== Change ChromiumCommit.format_patch to use a filtered list of files (2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 Review-Url: https://codereview.chromium.org/2576353003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change ChromiumCommit.format_patch to use a filtered list of files (2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 Review-Url: https://codereview.chromium.org/2576353003 ========== to ========== Change ChromiumCommit.format_patch to use a filtered list of files (2/5) This CL is based off of https://codereview.chromium.org/2579193002 and needs to be rebased once that is landed. BUG=657117,671890 Committed: https://crrev.com/4a865436e0b0437e17772c8b8828c77b55e7328e Cr-Commit-Position: refs/heads/master@{#440301} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4a865436e0b0437e17772c8b8828c77b55e7328e Cr-Commit-Position: refs/heads/master@{#440301} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
