Bug 89470 - webkitpy: Make webkit-patch patches-to-review useful
Summary: webkitpy: Make webkit-patch patches-to-review useful
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-19 07:39 PDT by Thiago Marcos P. Santos
Modified: 2012-06-26 11:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2012-06-19 07:47 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch v2 (11.31 KB, patch)
2012-06-19 12:13 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-06-19 07:39:47 PDT
Currently the tool just output a list of attachment ID pending review.

This refactoring will make it output a list of bugs that has a developer (defaults to the bugzilla username) on the CC, sorted by how old is the patch and excluding patches marked as r? but had cq denied by the bot.

$ ./Tools/Scripts/webkit-patch patches-to-review
Logging in as username@email.com...
Bugs with attachments pending review that has username@email.com on CC:
http://wkb.ug/bugid Description (age in days)
http://wkb.ug/86310 [GStreamer] media/media-continues-playing-after-replace-source.html fails (5)
http://wkb.ug/86615 [EFL][DRT] EFL DRT needs deletebutton controller (8)
http://wkb.ug/82864 [EFL] LayoutTestController needs implementation of setTabKeyCyclesThroughElements (20)
http://wkb.ug/84589 [EFL][DRT] Implementation of showModalDialog needed (41)
Total: 4
$

Configurable options for this first version:

$ ./Tools/Scripts/webkit-patch help patches-to-review
patches-to-review [options]   List bugs that has attachments pending review

Options:
  --all                Show all bugs regardless of who is on CC (it might take
                       a while)
  --include-cq-denied  By default, r? patches with cq- are omitted unless this
                       option is set
  --cc-email=CC_EMAIL  Specifies the email on the CC field (defaults to your
                       bugzilla login email)
Comment 1 Thiago Marcos P. Santos 2012-06-19 07:47:28 PDT
Created attachment 148332 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-06-19 08:51:58 PDT
Comment on attachment 148332 [details]
Patch

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

I really like this change.  But I think we should go one more round here.  Thanks!

> Tools/Scripts/webkitpy/tool/commands/queries.py:127
> +    help_text = "List bugs that has attachments pending review"

"List bugs which have attachments pending review".  Mostly "have" is the important part.  Not sure about which vs. that here.

> Tools/Scripts/webkitpy/tool/commands/queries.py:134
> +            make_option("--all", action="store_true", default=False,
> +                        help="Show all bugs regardless of who is on CC (it might take a while)"),
> +            make_option("--include-cq-denied", action="store_true", default=False,
> +                        help="By default, r? patches with cq- are omitted unless this option is set"),

Similarly, I expect the default=False aren't needed here either.  They don't hurt though. :)

> Tools/Scripts/webkitpy/tool/commands/queries.py:135
> +            make_option("--cc-email", default=None,

I believe "default" is optional here?  I believe action defaults to "store" and default defaults to None.

> Tools/Scripts/webkitpy/tool/commands/queries.py:141
> +        print "http://wkb.ug/bugid Description (age in days)"

I might suggest using more than a single space as a field delimeter here.  Similar to how you might want to have slightly smarter field printing as discussed below.  I suspect python has some fancy stuff for field printing built in, but I'm not sure.

> Tools/Scripts/webkitpy/tool/commands/queries.py:144
> +        for row in report:
> +            print "%s (%d)" % (row[1], row[0])

Do you want to limit the strings?  Also normally you want the variable-length field to be last?  You'll probably want to set a fixed-width for the URL field, since we have bug numbers ranging in size from 4 digits all the way up to 6 these days.

> Tools/Scripts/webkitpy/tool/commands/queries.py:152
> +            credentials = Credentials(config_urls.bug_server_host, git_prefix="bugzilla")

Do we have these on the Host object somewhere?  I'm a little surprised you'd have to do this yourself?  Maybe on host.scm.credentials?

> Tools/Scripts/webkitpy/tool/commands/queries.py:153
> +        cc_email = options.cc_email
> +
> +        if not cc_email and not options.all:
> +            credentials = Credentials(config_urls.bug_server_host, git_prefix="bugzilla")
> +            cc_email = credentials.read_credentials()[0]

I would have probably put thsi whole credential lookup stuff into a helper function.  cc_email = self._lookup_user_email(options) or similar.

> Tools/Scripts/webkitpy/tool/commands/queries.py:160
> +        else:
> +            print "Bugs with attachments pending review that has %s on CC:" % cc_email

I would probably write that as "in the CC list:" instead of "on CC:"  or "which has %s CC'd:"  I'm not sure if "which" or "that" is more grammatically correct here (despite being a native english speaker... sorry.)

> Tools/Scripts/webkitpy/tool/commands/queries.py:170
> +        report = []
> +        for bug in bugs:
> +            patch = bug.unreviewed_patches()[-1]
> +
> +            if not options.include_cq_denied and patch.commit_queue() == "-":
> +                continue
> +
> +            age_in_days = (datetime.today() - patch.attach_date()).days
> +            report.append((age_in_days, "http://wkb.ug/%s %s" % (bug.id(), bug.title())))

I would have put this in a helper function.
Comment 3 Dirk Pranke 2012-06-19 10:45:25 PDT
We might want to use bugs.webkit.org/b/12345 rather than http://wkb.ug/12345 . I don't feel strongly about either, but I don't know who owns the latter URL and I don't know how widely we encourage it?
Comment 4 Thiago Marcos P. Santos 2012-06-19 12:13:00 PDT
Created attachment 148387 [details]
Patch v2

Thanks for reviewing. I moved all the printing logic to a separated method, so as the the report generation logic. Replaced the short bug URL with the one within WK domains. Also added the padding trick for the bug url column (not exactly at the same line as it was suggest by the review). A new utest was added to verify if the default username is the same as the bugzilla username.
Comment 5 Dirk Pranke 2012-06-20 12:55:41 PDT
looks pretty good to me. Eric, do you want to take another look?
Comment 6 Tim Horton 2012-06-21 01:04:40 PDT
(In reply to comment #0)
> excluding patches marked as r? but had cq denied by the bot.

Why is this? It seems like maybe *some* indication that that is the case might be useful, but leaving those bugs out altogether doesn't quite seem right, as patches get cq-'d by the bots for pretty silly reasons sometimes.
Comment 7 Thiago Marcos P. Santos 2012-06-21 01:33:38 PDT
(In reply to comment #6)
> (In reply to comment #0)
> > excluding patches marked as r? but had cq denied by the bot.
> 
> Why is this? It seems like maybe *some* indication that that is the case might be useful, but leaving those bugs out altogether doesn't quite seem right, as patches get cq-'d by the bots for pretty silly reasons sometimes.

Because it is not always right, you can remove this filter by using:
--include-cq-denied
Comment 8 Eric Seidel (no email) 2012-06-26 10:33:46 PDT
Comment on attachment 148387 [details]
Patch v2

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

Seems fine.

> Tools/Scripts/webkitpy/tool/commands/queries.py:167
> +        tool.bugs.authenticate()

Does this command still work if they don't have bugzilla setup in their git config?
Comment 9 Thiago Marcos P. Santos 2012-06-26 11:04:26 PDT
(In reply to comment #8)
> (From update of attachment 148387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148387&action=review
> 
> Seems fine.
> 
> > Tools/Scripts/webkitpy/tool/commands/queries.py:167
> > +        tool.bugs.authenticate()
> 
> Does this command still work if they don't have bugzilla setup in their git config?

Yes, you get a prompt asking for the bugzilla login/password.
Comment 10 WebKit Review Bot 2012-06-26 11:27:59 PDT
Comment on attachment 148387 [details]
Patch v2

Clearing flags on attachment: 148387

Committed r121269: <http://trac.webkit.org/changeset/121269>
Comment 11 WebKit Review Bot 2012-06-26 11:28:15 PDT
All reviewed patches have been landed.  Closing bug.