Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1901733003:
git_cl: description fetching from code review servers. (Closed)
|
Created:
4 years, 8 months ago by martiniss Modified:
4 years, 8 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: description fetching from code review servers.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300207
Patch Set 1 #
Total comments: 11
Patch Set 2 : Simplified a lot yay. #
Total comments: 14
Patch Set 3 : Simplify more, tests. #
Total comments: 2
Patch Set 4 : Small whitespace. #Messages
Total messages: 18 (6 generated)
martiniss@chromium.org changed reviewers: + agable@chromium.org, tandrii@chromium.org
PTAL Not sure if it's the best but it does work! Still need tests though probably, right
approach - sgtm. look into tests/git_cl_test.py and create a new test akin to git cl patch or smth like that.
+comments https://codereview.chromium.org/1901733003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode2524 git_cl.py:2524: def _add_codereview_select_options(parser): for consistency, please use this func and the one below it. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3260 git_cl.py:3260: help='The issue to fetch the description from') s/from/from, instead of the current branch issue. or maybe even better way to put it so default is obvious? also, it might be easier for users to just copy-paste URL from browser, and expect it to work, similar to git cl patch. The codereview server will them be inferred from it. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3265 git_cl.py:3265: parser.add_option('-s', '--server', do you actually use this field? If you do, then make use of it for gerrit as well + document what this is - url or hostname, unless both are OK. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3271 git_cl.py:3271: if options.gerrit and options.rietveld: see _process_codereview_select_options https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3275 git_cl.py:3275: DieWithError('one of --gerrit and --rietveld is required') why is this so? it wasn't required before. and the codereview server can be guessed from local repo (settings, or remote url, or whatever). https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3292 git_cl.py:3292: # else nothing, because gerrit guesses the code review from the git host, for so does rietveld, as it's the server is recorded in codereview.settings.
Ok, code is much simpler now. Still need to add more tests and make it pass, but I figured out how to make one pass (test_description_display, I believe). https://codereview.chromium.org/1901733003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3260 git_cl.py:3260: help='The issue to fetch the description from') On 2016/04/18 at 22:13:16, tandrii(chromium) wrote: > s/from/from, instead of the current branch issue. > > or maybe even better way to put it so default is obvious? > also, it might be easier for users to just copy-paste URL from browser, and expect it to work, similar to git cl patch. The codereview server will them be inferred from it. Changed. And made it so you can copy paste a URL. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3265 git_cl.py:3265: parser.add_option('-s', '--server', On 2016/04/18 at 22:13:16, tandrii(chromium) wrote: > do you actually use this field? If you do, then make use of it for gerrit as well + document what this is - url or hostname, unless both are OK. Removed. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3271 git_cl.py:3271: if options.gerrit and options.rietveld: On 2016/04/18 at 22:13:16, tandrii(chromium) wrote: > see _process_codereview_select_options Done. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3275 git_cl.py:3275: DieWithError('one of --gerrit and --rietveld is required') On 2016/04/18 at 22:13:16, tandrii(chromium) wrote: > why is this so? it wasn't required before. and the codereview server can be guessed from local repo (settings, or remote url, or whatever). Removed. https://codereview.chromium.org/1901733003/diff/1/git_cl.py#newcode3292 git_cl.py:3292: # else nothing, because gerrit guesses the code review from the git host, for On 2016/04/18 at 22:13:16, tandrii(chromium) wrote: > so does rietveld, as it's the server is recorded in codereview.settings. Removed.
https://codereview.chromium.org/1901733003/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3255 git_cl.py:3255: @subcommand.usage('<codereview url or issue id>') wrap in [] to indicate optionality. by default, issue associated with branch is used. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3260 git_cl.py:3260: parser.add_option('-i', '--issue', this contradicts the help above. IMO, choose either "-i <coderview url or issue id> " or actual position arg. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3274 git_cl.py:3274: target_issue = issue_arg.issue you are potentially disregarding the codereview server here, but IMO, it's OK. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3283 git_cl.py:3283: cl = Changelist(**kwargs) you don't need kwargs var any more. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3288 git_cl.py:3288: print >> sys.stdout, description.description debug leftover? https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1381: real_out = sys.stdout remove this https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1398: real_out = sys.stdout ditto https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1406: ((['git', 'config', 'branch.feature.rietveldissue'],), '123123'), this is wrong. This means that coincidentally local branch has already issue 123123 associated. This call should never be made if '-i' flag is given. https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1412: 'description', '-d', '-i', '123123', '-s', 'https://code.review.org' ah, this is outdated test.
Ok, more fixes, should be good to go. PTAL https://codereview.chromium.org/1901733003/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3255 git_cl.py:3255: @subcommand.usage('<codereview url or issue id>') On 2016/04/19 at 09:42:04, tandrii(chromium) wrote: > wrap in [] to indicate optionality. by default, issue associated with branch is used. Done. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3260 git_cl.py:3260: parser.add_option('-i', '--issue', On 2016/04/19 at 09:42:04, tandrii(chromium) wrote: > this contradicts the help above. IMO, choose either "-i <coderview url or issue id> " or actual position arg. Removed. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3283 git_cl.py:3283: cl = Changelist(**kwargs) On 2016/04/19 at 09:42:04, tandrii(chromium) wrote: > you don't need kwargs var any more. Fixed. https://codereview.chromium.org/1901733003/diff/20001/git_cl.py#newcode3288 git_cl.py:3288: print >> sys.stdout, description.description On 2016/04/19 at 09:42:04, tandrii(chromium) wrote: > debug leftover? No, you need this to be able to mock it in the tests. https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1901733003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1412: 'description', '-d', '-i', '123123', '-s', 'https://code.review.org' On 2016/04/19 at 09:42:04, tandrii(chromium) wrote: > ah, this is outdated test. Yup. Tests should be good to go now.
LGTM % comments https://codereview.chromium.org/1901733003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1901733003/diff/40001/git_cl.py#newcode3289 git_cl.py:3289: print >> sys.stdout, description.description well, but print always goes to sys.stdout, so i find this redundant. Am I wrong/have i missed something? https://codereview.chromium.org/1901733003/diff/40001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1901733003/diff/40001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1404: self.mock(git_cl.Changelist, 'GetDescription', if you have diff tests for Gerrit/Rietveld, then actually mock FetchDescription of the respective implementation classes. That said, I personally would just have kept the Gerrit test below only :)
On 2016/04/26 at 10:34:20, tandrii wrote: > LGTM % comments > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py#newcode3289 > git_cl.py:3289: print >> sys.stdout, description.description > well, but print always goes to sys.stdout, so i find this redundant. Am I wrong/have i missed something? I did this so I could mock out the print, and capture it in my test, to make sure it prints the correct thing. > > https://codereview.chromium.org/1901733003/diff/40001/tests/git_cl_test.py > File tests/git_cl_test.py (right): > > https://codereview.chromium.org/1901733003/diff/40001/tests/git_cl_test.py#ne... > tests/git_cl_test.py:1404: self.mock(git_cl.Changelist, 'GetDescription', > if you have diff tests for Gerrit/Rietveld, then actually mock FetchDescription of the respective implementation classes. That said, I personally would just have kept the Gerrit test below only :)
The CQ bit was checked by martiniss@chromium.org
The CQ bit was unchecked by martiniss@chromium.org
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/1901733003/#ps60001 (title: "Small whitespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1901733003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1901733003/60001
Message was sent while issue was closed.
Description was changed from ========== git_cl: description fetching from code review servers. BUG= ========== to ========== git_cl: description fetching from code review servers. BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300207 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300207
Message was sent while issue was closed.
On 2016/04/26 20:24:39, martiniss wrote: > On 2016/04/26 at 10:34:20, tandrii wrote: > > LGTM % comments > > > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py > > File git_cl.py (right): > > > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py#newcode3289 > > git_cl.py:3289: print >> sys.stdout, description.description > > well, but print always goes to sys.stdout, so i find this redundant. Am I > wrong/have i missed something? > > I did this so I could mock out the print, and capture it in my test, to make > sure it prints the correct thing. Well, yes, I saw your test. Yet my point is that a) >> sys.stdout is redundant as in your test would still work. Proof: https://codereview.chromium.org/1924643002 b) wasn't yet used in this codebase. So, why do that?
Message was sent while issue was closed.
On 2016/04/26 at 21:22:04, tandrii wrote: > On 2016/04/26 20:24:39, martiniss wrote: > > On 2016/04/26 at 10:34:20, tandrii wrote: > > > LGTM % comments > > > > > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py > > > File git_cl.py (right): > > > > > > https://codereview.chromium.org/1901733003/diff/40001/git_cl.py#newcode3289 > > > git_cl.py:3289: print >> sys.stdout, description.description > > > well, but print always goes to sys.stdout, so i find this redundant. Am I > > wrong/have i missed something? > > > > I did this so I could mock out the print, and capture it in my test, to make > > sure it prints the correct thing. > Well, yes, I saw your test. Yet my point is that > a) >> sys.stdout is redundant as in your test would still work. Proof: https://codereview.chromium.org/1924643002 > b) wasn't yet used in this codebase. So, why do that? Oh whoops :( I tried that once and it didn't work, my bad. |
