[webkit-reviews] review denied: [Bug 56989] webkit-patch should be more intelligent about whether a bug applies to a patch : [Attachment 86875] Proposed patch to improve parsing of bug IDs from changelogs

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Andrew Foster
<andrewf at chromium.org>'s request for review:
Bug 56989: webkit-patch should be more intelligent about whether a bug applies
to a patch
https://bugs.webkit.org/show_bug.cgi?id=56989

Attachment 86875: Proposed patch to improve parsing of bug IDs from changelogs
https://bugs.webkit.org/attachment.cgi?id=86875&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86875&action=review

The regexp and tests look good. Just not sure about the need for an argument.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:53
> +def parse_bug_id(message, show_bug_url_on_own_line=False):

I don't see why you need this argument. As best I can tell, all the paces where
this is used are to get the bug ID out of a changelog/commit entry and they
should all have the same format. Is there a case that breaks if you just always
restrict to searching for the bug url on it's own line?

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:59
> +    if show_bug_url_on_own_line == False:

If you invert the order here, you can just check "if show_bug_url_on_own_line".
Also, general preferred style is to use "if bool" or "if not bool" instead of
comparing to True/False.