Bug 57184 - check-webkit-style should check ChangeLog for a valid bug number
Summary: check-webkit-style should check ChangeLog for a valid bug number
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
: 37943 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-03-27 09:15 PDT by Patrick R. Gansterer
Modified: 2011-03-29 10:12 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.85 KB, patch)
2011-03-27 09:16 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (14.12 KB, patch)
2011-03-27 19:14 PDT, Patrick R. Gansterer
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-03-27 09:15:15 PDT
check-webkit-style should check ChangeLog for a valid bug number
Comment 1 Patrick R. Gansterer 2011-03-27 09:16:40 PDT
Created attachment 87074 [details]
Patch
Comment 2 David Levin 2011-03-27 13:22:42 PDT
Comment on attachment 87074 [details]
Patch

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@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
Comment 3 Patrick R. Gansterer 2011-03-27 19:14:59 PDT
Created attachment 87096 [details]
Patch
Comment 4 David Levin 2011-03-27 19:26:09 PDT
Comment on attachment 87096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87096&action=review

Awesome. Thanks!

> Tools/Scripts/webkitpy/style/checkers/changelog.py:43
> +        for line_number, line in enumerate(entry_lines):

get rid of enumerate here.

> Tools/Scripts/webkitpy/style/checkers/changelog.py:63
> +                if line_number == 0:

get rid of comparison to 0.
Comment 5 Patrick R. Gansterer 2011-03-27 19:33:39 PDT
Committed r82083: <http://trac.webkit.org/changeset/82083>
Comment 6 David Kilzer (:ddkilzer) 2011-03-29 10:12:48 PDT
*** Bug 37943 has been marked as a duplicate of this bug. ***