[Webkit-unassigned] [Bug 27172] bugzilla-tool: create CommitMessage class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27172





--- Comment #4 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2009-07-15 17:15:13 PDT ---
(In reply to comment #1)
> (From update of attachment 32604 [details])
> 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?)

After thinking about this over the weekend, I do think a CommitMessage object
should contain a list of ChangeLogEntry objects.  It could then use these
objects to create a commit message string.

> 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()

Yes.

> 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?

This would strip the URL" off the beginning of a bug description, e.g.:

<rdar://problem/1234567> This is a serious bug
<http://webkit.org/b/12345> This is an even more serious bug

The new "webkit.org/b/NNNNN" URL was discussed on webkit-dev recently.

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

Yes, per above discussion.

> 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)

The first one parses out the shortened WebKit bug URL format
("webkit.org/b/NNNNN").  The second parses out the bugs.webkit.org URL format.

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

None is returned implicitly if there is no return statement?

> 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.

Brent committed this before I had a chance to clear the review flag.  (It
doesn't hurt that it landed, though.  I can continue refactoring CommitMessage
to use ChangeLogEntry objects.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.