[webkit-reviews] review granted: [Bug 27172] bugzilla-tool: create CommitMessage class : [Attachment 32604] Patch v1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 27172: bugzilla-tool: create CommitMessage class
https://bugs.webkit.org/show_bug.cgi?id=27172

Attachment 32604: Patch v1
https://bugs.webkit.org/attachment.cgi?id=32604&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
It's difficult for me to tell where the divide between a CommitMessage class
and ChangeLog* classes should be.

 114	 return CommitMessage(''.join(changelog_messages).splitlines())
Seems a little odd to take an array of lines, but OK.

Maybe CommitMessage should take an array of ChangeLog paths instead?
(eventually probably an array of ChangeLog entries?)

Should we name this bug_id and have it lazily call parse_bug_id?
 426		 bug_id = options.bug_id or commit_message.parse_bug_id()

I don't understand this code:
 72	    if strip_url:
 73		line = re.sub("^(\s*)<.+> ", "\1", line)

Seems it at least needs a comment.  I assume this is some new prepare-ChangeLog
thing which I'm not used to yet?

Some of this code really seems to belong in a ChangeLogEntry class.

Seems odd that these lines don't match:
 81		match = re.search("http\://webkit\.org/b/(?P<bug_id>\d+)",
line)
 84		match = re.search(Bugzilla.bug_server_regex +
"show_bug\.cgi\?id=(?P<bug_id>\d+)", line)

I don't think this is needed:
 87	    return None

I think this is OK.  But you might want to try some of the comments above.  I
think much of this will end up in a ChangeLogEntry class instead of in
CommitMessage.	CommitMessage seems useful for doing all the commit-log-editor
stuff dealing with ChangeLogEntry (or similar) classes.