Issue 6719004: refactor parsing of change descriptions, fix TBR=

Issue 6719004: refactor parsing of change descriptions, fix TBR= (Closed)

Created:
9 years, 9 months ago by Dirk Pranke

Modified:
9 years, 7 months ago

Reviewers:
M-A Ruel

CC:
chromium-reviews, M-A Ruel, Lei Zhang, Stephen White

Base URL:
svn://svn.chromium.org/chrome/trunk/tools/depot_tools

Visibility:
Public.

Description

The parsing of change descriptions had a lot of overlap and inconsistencies between gcl and git-cl. In particular, we weren't handling TBR= consistently, or probably a few other things. This change moves most of the code into presubmit_support and gclient_utils and just leaves the formatting differences for the messages between the two tools. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79002

Patch Set 1 #

Patch Set 2 : refactor, clean up, test, etc. #

Patch Set 3 : remove TestableChangeDescription; the base class is already testable #

Total comments: 4

Patch Set 4 : rebase to head, fix bugs #

Created: 9 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+277 lines, -191 lines)			Patch
M	gcl.py	View	1 2 3	7 chunks	+28 lines, -69 lines	0 comments	Download
M	gclient_utils.py	View	1 2 3	2 chunks	+35 lines, -0 lines	0 comments	Download
M	git_cl/git_cl.py	View	1 2 3	11 chunks	+40 lines, -114 lines	0 comments	Download
M	presubmit_support.py	View	1 2 3	1 chunk	+90 lines, -0 lines	0 comments	Download
M	tests/gcl_unittest.py	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	tests/gclient_utils_test.py	View	1	1 chunk	+5 lines, -5 lines	0 comments	Download
M	tests/presubmit_unittest.py	View	1 2 3	3 chunks	+78 lines, -2 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

M-A Ruel

lgtm with nits. http://codereview.chromium.org/6719004/diff/4001/gcl.py File gcl.py (right): http://codereview.chromium.org/6719004/diff/4001/gcl.py#newcode1093 gcl.py:1093: text = change_desc.EditableDescription() + footer Why ...

9 years, 9 months ago (2011-03-22 17:24:16 UTC) #2

Dirk Pranke

9 years, 9 months ago (2011-03-22 18:08:31 UTC) #3

On 2011/03/22 17:24:16, Marc-Antoine Ruel wrote:
> lgtm with nits.
> 
> http://codereview.chromium.org/6719004/diff/4001/gcl.py
> File gcl.py (right):
> 
> http://codereview.chromium.org/6719004/diff/4001/gcl.py#newcode1093
> gcl.py:1093: text = change_desc.EditableDescription() + footer
> Why not?
> result = change_desc...
> if not silent:
>   result = change_desc.editor(result)
> 

Good idea. Done.

> http://codereview.chromium.org/6719004/diff/4001/gclient_utils.py
> File gclient_utils.py (right):
> 
> http://codereview.chromium.org/6719004/diff/4001/gclient_utils.py#newcode745
> gclient_utils.py:745: text = FileRead(filename, 'r')
> you can return right here, no need for 'text' variable.
>

Done.
 
> http://codereview.chromium.org/6719004/diff/4001/presubmit_support.py
> File presubmit_support.py (right):
> 
>
http://codereview.chromium.org/6719004/diff/4001/presubmit_support.py#newcode815
> presubmit_support.py:815: class ChangeDescription(object):
> It's a slightly odd location for this class but it's fine.
>

I feel like Change and ChangeDescription should probably be pulled into their
own class, but that's a separate CL.
 
> http://codereview.chromium.org/6719004/diff/4001/tests/presubmit_unittest.py
> File tests/presubmit_unittest.py (right):
> 
>
http://codereview.chromium.org/6719004/diff/4001/tests/presubmit_unittest.py#...
> tests/presubmit_unittest.py:1974: 
> only 2 lines.

Done.

Thanks!

Expand Messages | Collapse Messages