Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922133006:
git_cl: Add the ability to set the description. (Closed)
|
Created:
4 years, 7 months ago by martiniss Modified:
4 years, 7 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, tandrii+omg_git_cl_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit_cl: Add the ability to set the description.
BUG=607359
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300357
Patch Set 1 #
Total comments: 4
Patch Set 2 : One line, moved changelist mock out. #
Total comments: 4
Patch Set 3 : nits for tests. #Messages
Total messages: 17 (7 generated)
Description was changed from ========== git_cl: Add the ability to set the description. BUG= ========== to ========== git_cl: Add the ability to set the description. BUG=607359 ==========
martiniss@chromium.org changed reviewers: + iannucci@chromium.org, tandrii@chromium.org
PTAL
LGTM % comment to reduce copy-pasting https://codereview.chromium.org/1922133006/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1922133006/diff/1/git_cl.py#newcode3307 git_cl.py:3307: text += line + '\n' one liner instead of three is faster: text = '\n'.join(sys.stdin.splitlines()) https://codereview.chromium.org/1922133006/diff/1/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1922133006/diff/1/tests/git_cl_test.py#newcod... tests/git_cl_test.py:1388: class MockChangelist(): i think it's time to kick this outside of this this method and share it for 3 tests.
PTAL https://codereview.chromium.org/1922133006/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1922133006/diff/1/git_cl.py#newcode3307 git_cl.py:3307: text += line + '\n' On 2016/04/28 at 05:13:56, tandrii(chromium) wrote: > one liner instead of three is faster: > > text = '\n'.join(sys.stdin.splitlines()) Fixed. I had to make another mock in the tests, so PTAL again. https://codereview.chromium.org/1922133006/diff/1/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1922133006/diff/1/tests/git_cl_test.py#newcod... tests/git_cl_test.py:1388: class MockChangelist(): On 2016/04/28 at 05:13:56, tandrii(chromium) wrote: > i think it's time to kick this outside of this this method and share it for 3 tests. Done.
Still LGTM, but still a comment for tests :) https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:24: class ChangelistMock(): nit: add (object) https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1455: self.mock(git_cl.sys, 'stdin', TMP()) why not just 'hi\nthere'? IMO, best futureproof for streams is StringIO.StringIO('hi\nthree')
https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:24: class ChangelistMock(): On 2016/04/29 at 15:37:41, tandrii(chromium) wrote: > nit: add (object) Done. https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1455: self.mock(git_cl.sys, 'stdin', TMP()) On 2016/04/29 at 15:37:40, tandrii(chromium) wrote: > why not just 'hi\nthere'? IMO, best futureproof for streams is > StringIO.StringIO('hi\nthree') Sadly, stringio doesn't have a splitlines :( I think this is why I had the weird code in git_cl.py in the first place. I'll just stick with this small class for now.
The CQ bit was checked by martiniss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/1922133006/#ps40001 (title: "nits for tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922133006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922133006/40001
Message was sent while issue was closed.
Description was changed from ========== git_cl: Add the ability to set the description. BUG=607359 ========== to ========== git_cl: Add the ability to set the description. BUG=607359 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300357 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300357
Message was sent while issue was closed.
On 2016/04/29 17:10:46, martiniss wrote: > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py > File tests/git_cl_test.py (right): > > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... > tests/git_cl_test.py:24: class ChangelistMock(): > On 2016/04/29 at 15:37:41, tandrii(chromium) wrote: > > nit: add (object) > > Done. > > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... > tests/git_cl_test.py:1455: self.mock(git_cl.sys, 'stdin', TMP()) > On 2016/04/29 at 15:37:40, tandrii(chromium) wrote: > > why not just 'hi\nthere'? IMO, best futureproof for streams is > > StringIO.StringIO('hi\nthree') > > Sadly, stringio doesn't have a splitlines :( I think this is why I had the weird > code in git_cl.py in the first place. I'll just stick with this small class for > now. oops, yes, you are right.
Message was sent while issue was closed.
On 2016/04/29 17:55:25, tandrii(chromium) wrote: > On 2016/04/29 17:10:46, martiniss wrote: > > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py > > File tests/git_cl_test.py (right): > > > > > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:24: class ChangelistMock(): > > On 2016/04/29 at 15:37:41, tandrii(chromium) wrote: > > > nit: add (object) > > > > Done. > > > > > https://codereview.chromium.org/1922133006/diff/20001/tests/git_cl_test.py#ne... > > tests/git_cl_test.py:1455: self.mock(git_cl.sys, 'stdin', TMP()) > > On 2016/04/29 at 15:37:40, tandrii(chromium) wrote: > > > why not just 'hi\nthere'? IMO, best futureproof for streams is > > > StringIO.StringIO('hi\nthree') > > > > Sadly, stringio doesn't have a splitlines :( I think this is why I had the > weird > > code in git_cl.py in the first place. I'll just stick with this small class > for > > now. > oops, yes, you are right. Argh, I'm double wrong. sys.stdin is just like StringIO - neither has splitlines. Test your code "live" and you'll see: tandrii@andrii:0:~/bin/depot_tools$ git cl description -n - Traceback (most recent call last): File "/usr/local/google/home/tandrii/bin/depot_tools/git_cl.py", line 4883, in <module> sys.exit(main(sys.argv[1:])) File "/usr/local/google/home/tandrii/bin/depot_tools/git_cl.py", line 4865, in main return dispatcher.execute(OptionParser(), argv) File "/usr/local/google/home/tandrii/bin/depot_tools/subcommand.py", line 252, in execute return command(parser, args[1:]) File "/usr/local/google/home/tandrii/bin/depot_tools/git_cl.py", line 3315, in CMDdescription text = '\n'.join(sys.stdin.splitlines()) AttributeError: 'file' object has no attribute 'splitlines' So, it should be: text = '\n'.join(l.rstrip() for l in sys.stdin and then StringsIO will be a perfect mock indeed.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1935633002/ by martiniss@chromium.org. The reason for reverting is: splitlines man.
Message was sent while issue was closed.
Description was changed from ========== git_cl: Add the ability to set the description. BUG=607359 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300357 ========== to ========== git_cl: Add the ability to set the description. BUG=607359 ==========
Message was sent while issue was closed.
Description was changed from ========== git_cl: Add the ability to set the description. BUG=607359 ========== to ========== git_cl: Add the ability to set the description. BUG=607359 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300357 ========== |
