[webkit-reviews] review denied: [Bug 57184] check-webkit-style should check ChangeLog for a valid bug number : [Attachment 87074] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 57184: check-webkit-style should check ChangeLog for a valid bug number
https://bugs.webkit.org/show_bug.cgi?id=57184

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87074&action=review

Just a few things to address and this should be good to go!

Thanks.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:13
> +#	 * Neither the name of Google Inc. nor the names of its

I'm not sure if you want this 3rd clause in here, but if you want to keep it,
whatever :).

> Tools/Scripts/webkitpy/style/checkers/changelog.py:42
> +	   self.complain_bug_numer = False

typo: numer
Also this seems completely unused, so I would just remove it.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:50
> +	       line_number += entry_line_number

It looks like line_number is never used, so you can remove it and the enumerate
call here.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:52
> +	       if
re.search("https?://bugs.webkit.org/show_bug\.cgi\?id=\d+|http://webkit.org/b/\
d+", line):

Consider using parse_bug_id from webkitpy/common/net/bugzilla/bugzilla.py
instead.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:58
> +

Consider using a for else pattern here which feels like it fits better:

for ...
    if blah:
       break
else:
   error()

> Tools/Scripts/webkitpy/style/checkers/changelog.py:61
> +					       "changelog/bugnumber", 5,

I'd align with the ( from the previous line.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:62
> +					       "ChaneLog entry has no bug
number")

typo: ChaneLog
 (and the alignement issue).

> Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:29
> +

Why the blank line?

> Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:44
> +	       self.had_error = True

Where does this get set back to false?

> Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:76
> +	       '2011-01-01  Patrick Gansterer  <paroga at paroga.com>\n\n	     
Unreview build fix for r12345.\n',

Please add a test case like this:
https://bugs.webkit.org/attachment.cgi?id=74800&action=prettypatch