[webkit-reviews] review denied: [Bug 28477] WebKit needs a changelogs.py to hold changelog-related code : [Attachment 35167] Patch v1

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 28477: WebKit needs a changelogs.py to hold changelog-related code
https://bugs.webkit.org/show_bug.cgi?id=28477

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

------- Additional Comments from David Levin <levin at chromium.org>
A few things to consider..

> diff --git a/WebKitTools/Scripts/modules/changelogs.py
b/WebKitTools/Scripts/modules/changelogs.py
> +# Copyright (c) 2009, Google Inc. All rights reserved.
(C)

> +import fileinput # inplace file editing for set_reviewer_in_changelog
> +import re
> +

> +# This doesn't really belong in this file, but we don't have a better home
for it yet.
FIXME:

> +	       if ChangeLog.date_line_regexp.match(line):
> +		   return ''.join(entry_lines[0:-1]) # Remove the extra newline

at the end

Indexes at the boundary need not be listed, so it could be
 return ''.join(entry_lines[:-1]) # Remove the extra newline at the end.

However, I feel mildly paranoid about the blank line, so why not be slight more

robust line this?

 if not entry_lines[-1].strip():
     entry_lines = entry_lines[:-1]
 return ''.join(entry_lines) # Remove the extra newline at the end.



> +    def update_for_revert(self, revision):
> +	   reviewed_by_regexp = re.compile('Reviewed by NOBODY \(OOPS!\)\.')
> +	   removing_boilerplate = False
> +	   # inplace=1 creates a backup file and re-directs stdout to the file
> +	   for line in fileinput.FileInput(self.path, inplace=1):
> +	       if re.search(reviewed_by_regexp, line):
reviewed_by_regexp.search(...

> +		   print re.sub(reviewed_by_regexp, "No review, rolling out
r%s." % revision, line),
reviewed_by_regexp.sub(...



> +		   print "	  %s\n" % view_source_url(revision)
> +		   # Remove all the ChangeLog boiler plate between the Reviewed

by line and the first changed file.
s/boiler plate/boilerplate/