Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409223002:
git cl try: make code less Rietveld-specific. (Closed)
|
Created:
4 years, 2 months ago by tandrii(chromium) Modified:
4 years, 2 months ago CC:
chromium-reviews, tandrii+omg_git_cl_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl try: make code less Rietveld-specific.
This also adds first test to cover case of custom properties.
R=machenbach@chromium.org,sergiyb@chromium.org
BUG=599931
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/de281aee41f5fae08e9274341b7995d26af32a66
Patch Set 1 #Patch Set 2 : Nit. #
Total comments: 13
Patch Set 3 : Review. #Messages
Total messages: 19 (14 generated)
The CQ bit was checked by tandrii@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/31cd9614561b6410)
The CQ bit was checked by tandrii@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2409223002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode322 git_cl.py:322: category='git_cl_try', patchset=None): nit: indentation https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode328 git_cl.py:328: codereview_url = changelist.GetCodereviewServer() Maybe assign already above to call GetCodereviewServer() only once? https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode344 git_cl.py:344: codereview='gerrit' if changelist.IsGerrit() else 'rietveld', How about hiding this one-line-condition in another helper method in changelist? https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode2775 git_cl.py:2775: raise NotImplementedError() So, did it not work before for gerrit at all? If it did, then it would run now into this exceptions. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode4859 git_cl.py:4859: patchset = cl.GetMostRecentPatchset() Independent of this CL, I think the names could be improved, like GetRemotePatchset and GetLocalPatchset? The others are confusing. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode4864 git_cl.py:4864: 'By default, git cl try uses latest patchset from ' nit: the latest patchset
https://codereview.chromium.org/2409223002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode322 git_cl.py:322: category='git_cl_try', patchset=None): On 2016/10/12 10:15:22, machenbach (slow) wrote: > nit: indentation Done. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode328 git_cl.py:328: codereview_url = changelist.GetCodereviewServer() On 2016/10/12 10:15:22, machenbach (slow) wrote: > Maybe assign already above to call GetCodereviewServer() only once? Done. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode344 git_cl.py:344: codereview='gerrit' if changelist.IsGerrit() else 'rietveld', On 2016/10/12 10:15:22, machenbach (slow) wrote: > How about hiding this one-line-condition in another helper method in changelist? IMO, not worth it, and it's more readable this way. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode2775 git_cl.py:2775: raise NotImplementedError() On 2016/10/12 10:15:22, machenbach (slow) wrote: > So, did it not work before for gerrit at all? If it did, then it would run now > into this exceptions. see line 4759 https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode4759 git_cl.py:4759: ' git cl set-commit --dry-run') this still blocks Gerrit CLs from having git cl try. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode4859 git_cl.py:4859: patchset = cl.GetMostRecentPatchset() On 2016/10/12 10:15:22, machenbach (slow) wrote: > Independent of this CL, I think the names could be improved, like > GetRemotePatchset and GetLocalPatchset? The others are confusing. yes, I agree. I will create a follow up. https://codereview.chromium.org/2409223002/diff/20001/git_cl.py#newcode4864 git_cl.py:4864: 'By default, git cl try uses latest patchset from ' On 2016/10/12 10:15:22, machenbach (slow) wrote: > nit: the latest patchset Done.
The CQ bit was checked by tandrii@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
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 tandrii@chromium.org
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.
Description was changed from ========== git cl try: make code less Rietveld-specific. This also adds first test to cover case of custom properties. R=machenbach@chromium.org,sergiyb@chromium.org BUG=599931 ========== to ========== git cl try: make code less Rietveld-specific. This also adds first test to cover case of custom properties. R=machenbach@chromium.org,sergiyb@chromium.org BUG=599931 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/de281aee41f5fa... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/de281aee41f5fa... |
