Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028303006:
Refactor git_footers for later use in git cl. (Closed)
|
Created:
4 years, 6 months ago by tandrii(chromium) Modified:
4 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@footer Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionRefactor git_footers for later use in git cl.
Motivation: git_cl should start understanding Gerrit's git footers,
to avoid appending BUG= after Change-Id: Ixxx line.
R=sergiyb@chromium.org
BUG=614587
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300691
Patch Set 1 #Patch Set 2 : more tests #Patch Set 3 : more refactor #
Total comments: 27
Patch Set 4 : awesome review #Patch Set 5 : Updated patchset dependency #
Total comments: 2
Patch Set 6 : nit #Messages
Total messages: 31 (13 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/patch-status/2028303006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/1
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/patch-status/2028303006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/20001
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please also see my comments in https://codereview.chromium.org/2038673002/. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (left): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#oldcode70 git_footers.py:70: nit: keep the second line, these are top-level defintions https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode38 git_footers.py:38: # Read footers from bottom to top. Why so? https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode49 git_footers.py:49: parsed_footers correspond each line in footer_lines. Document the structure of each entry in parsed_footers or better yet - document return of value of the parse_footer function and here simply mention that it matches its output. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode92 git_footers.py:92: Bug|Issue|Test|Feature. (optional) nit: Bug, Issue, Test, Feature https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode94 git_footers.py:94: assert 'Change-Id' not in parse_footers(message) (optional) Should we really use assert here? This function looks like it's part of the public API of this module and AFAIK, exceptions is the right way to report incorrect usage by the callers. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode110 git_footers.py:110: assert key == normalize_name(key), 'Use normalized key' ditto (also optional) https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode116 git_footers.py:116: top_lines = ['', ''] Why two empty lines here? https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode118 git_footers.py:118: top_lines.append('') What if there was already an empty line in the end? Wouldn't we be adding too many empty lines? https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode125 git_footers.py:125: for i, (key, _) in reversed(list(enumerate(parsed_footers))): How about this? for i, key in enumerate(reversed(parsed_footers)): https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.py File tests/git_footers_test.py (right): https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.... tests/git_footers_test.py:44: (['Header', ''], ['Actual: footer'], [('Actual', 'footer')])) also add self.assertEqual(git_footers.split_footers('\nActual: footer'), ([''], ['Actual: footer'], [('Actual', 'footer')])) https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.... tests/git_footers_test.py:133: 'Top\n\nSome: footer\nKey: value') I would also add a test when the footer must be added somewhere in the middle, e.g. self.assertEqual( git_footers.add_footer('Top\n\nSome: footer\nOther: footer', 'Key', 'value', after_keys=['Some']), 'Top\n\nSome: footer\nKey: value\nOther: footer')
yyanagisawa@chromium.org changed reviewers: + yyanagisawa@chromium.org
https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode54 git_footers.py:54: if line == '' or line.isspace(): Is non footer and footer are always split with empty line?
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/patch-status/2028303006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/60001
thanks for reviews! https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (left): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#oldcode70 git_footers.py:70: On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > nit: keep the second line, these are top-level defintions Done. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode38 git_footers.py:38: # Read footers from bottom to top. On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > Why so? that's how git_footers always worked. But added a better comment. WDYT? https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode49 git_footers.py:49: parsed_footers correspond each line in footer_lines. On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > Document the structure of each entry in parsed_footers or better yet - document > return of value of the parse_footer function and here simply mention that it > matches its output. Done. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode54 git_footers.py:54: if line == '' or line.isspace(): On 2016/06/03 07:00:00, Yoshisato Yanagisawa wrote: > Is non footer and footer are always split with empty line? Yes https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode94 git_footers.py:94: assert 'Change-Id' not in parse_footers(message) On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > (optional) Should we really use assert here? This function looks like it's part > of the public API of this module and True, but > AFAIK, exceptions is the right way to > report incorrect usage by the callers. NO! Assert is perfect for https://en.wikipedia.org/wiki/Design_by_contract The whole point is that assert can be skipped in optimized build, just like DCHECK in C++. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode116 git_footers.py:116: top_lines = ['', ''] On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > Why two empty lines here? At the time of writing this, I was thinking of minor UI optimization, but you are right, not needed. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode118 git_footers.py:118: top_lines.append('') On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > What if there was already an empty line in the end? Wouldn't we be adding too > many empty lines? Good point! added test for that. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode125 git_footers.py:125: for i, (key, _) in reversed(list(enumerate(parsed_footers))): On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > How about this? > > for i, key in enumerate(reversed(parsed_footers)): meaning of `i` would be different. https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.py File tests/git_footers_test.py (right): https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.... tests/git_footers_test.py:44: (['Header', ''], ['Actual: footer'], [('Actual', 'footer')])) On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > also add > > self.assertEqual(git_footers.split_footers('\nActual: footer'), ([''], ['Actual: > footer'], [('Actual', 'footer')])) good idea. https://codereview.chromium.org/2028303006/diff/40001/tests/git_footers_test.... tests/git_footers_test.py:133: 'Top\n\nSome: footer\nKey: value') On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > I would also add a test when the footer must be added somewhere in the middle, > e.g. > > self.assertEqual( > git_footers.add_footer('Top\n\nSome: footer\nOther: footer', > 'Key', 'value', after_keys=['Some']), > 'Top\n\nSome: footer\nKey: value\nOther: footer') excellent!
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ optional __doc__ improvement suggestion https://codereview.chromium.org/2028303006/diff/40001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode38 git_footers.py:38: # Read footers from bottom to top. On 2016/06/03 09:16:17, tandrii(chromium) wrote: > On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > > Why so? > > that's how git_footers always worked. But added a better comment. WDYT? Thanks. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode94 git_footers.py:94: assert 'Change-Id' not in parse_footers(message) On 2016/06/03 09:16:17, tandrii(chromium) wrote: > On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > > (optional) Should we really use assert here? This function looks like it's > part > > of the public API of this module and > True, but > > AFAIK, exceptions is the right way to > > report incorrect usage by the callers. > NO! Assert is perfect for https://en.wikipedia.org/wiki/Design_by_contract > The whole point is that assert can be skipped in optimized build, just like > DCHECK in C++. Acknowledged. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode116 git_footers.py:116: top_lines = ['', ''] On 2016/06/03 09:16:17, tandrii(chromium) wrote: > On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > > Why two empty lines here? > > At the time of writing this, I was thinking of minor UI optimization, but you > are right, not needed. Acknowledged. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode118 git_footers.py:118: top_lines.append('') On 2016/06/03 09:16:16, tandrii(chromium) wrote: > On 2016/06/02 23:16:26, Sergiy Byelozyorov wrote: > > What if there was already an empty line in the end? Wouldn't we be adding too > > many empty lines? > > Good point! added test for that. Acknowledged. https://codereview.chromium.org/2028303006/diff/40001/git_footers.py#newcode125 git_footers.py:125: for i, (key, _) in reversed(list(enumerate(parsed_footers))): On 2016/06/03 09:16:16, tandrii(chromium) wrote: > On 2016/06/02 23:16:27, Sergiy Byelozyorov wrote: > > How about this? > > > > for i, key in enumerate(reversed(parsed_footers)): > > meaning of `i` would be different. Actually yes, I forgot to delete my comment after testing it myself. https://codereview.chromium.org/2028303006/diff/80001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/80001/git_footers.py#newcode26 git_footers.py:26: """Returns footer's key and value if footer is valid, else None.""" Just to make it more clear that it's a tuple: Returns (footer_key, value) if footer is valid, else None. Btw, using inconsistent output makes it hard to use the function. I'd always return 2-tuple and return (None, None) in second case. But up to you.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yyanagisawa@chromium.org, sergiyb@chromium.org Link to the patchset: https://codereview.chromium.org/2028303006/#ps100001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028303006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028303006/100001
https://codereview.chromium.org/2028303006/diff/80001/git_footers.py File git_footers.py (right): https://codereview.chromium.org/2028303006/diff/80001/git_footers.py#newcode26 git_footers.py:26: """Returns footer's key and value if footer is valid, else None.""" On 2016/06/03 12:43:07, Sergiy Byelozyorov wrote: > Just to make it more clear that it's a tuple: > > Returns (footer_key, value) if footer is valid, else None. > > Btw, using inconsistent output makes it hard to use the function. I'd always > return 2-tuple and return (None, None) in second case. But up to you. Done.
Message was sent while issue was closed.
Description was changed from ========== Refactor git_footers for later use in git cl. Motivation: git_cl should start understanding Gerrit's git footers, to avoid appending BUG= after Change-Id: Ixxx line. R=sergiyb@chromium.org BUG=614587 ========== to ========== Refactor git_footers for later use in git cl. Motivation: git_cl should start understanding Gerrit's git footers, to avoid appending BUG= after Change-Id: Ixxx line. R=sergiyb@chromium.org BUG=614587 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300691 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300691 |
