Bug 89470

Created attachment 148332 [details] Patch

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.

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?

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.

looks pretty good to me. Eric, do you want to take another look?

(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.

(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 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?

(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 on attachment 148387 [details] Patch v2 Clearing flags on attachment: 148387 Committed r121269: <http://trac.webkit.org/changeset/121269>

All reviewed patches have been landed. Closing bug.