[webkit-reviews] review denied: [Bug 89470] webkitpy: Make webkit-patch patches-to-review useful : [Attachment 148332] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel (OOO until 6/19) <eric at webkit.org> has denied Thiago Marcos P.
Santos <tmpsantos at gmail.com>'s request for review:
Bug 89470: webkitpy: Make webkit-patch patches-to-review useful
https://bugs.webkit.org/show_bug.cgi?id=89470

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

------- Additional Comments from Eric Seidel (OOO until 6/19) <eric at webkit.org>
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.