[webkit-reviews] review denied: [Bug 27119] bugzilla-tool: Add create-bug command : [Attachment 33683] Patch v4

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 27119: bugzilla-tool: Add create-bug command
https://bugs.webkit.org/show_bug.cgi?id=27119

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

------- Additional Comments from David Levin <levin at chromium.org>
Biggest concern:

I don't understand the bootstrapping aspect to this. In order to create a well
formed patch, I need a changelog with a bug link/title.  This tool creates a
bug for a patch, but by definition, the patch must not be well formed because
the bug doesn't exist yet.

It could modify the ChangeLog right after creating the bug (but that sounds
complicated especially considering that you can given it a COMMITISH....)

On to the details.


> diff --git a/WebKitTools/Scripts/bugzilla-tool
b/WebKitTools/Scripts/bugzilla-tool
> +class CreateBug(Command):
> +    def __init__(self):
> +	   Command.__init__(self, 'Create a bug from local changes or local
commits', '[COMMITISH]', options=options)

Nice to add period to this sentence.

> +
> +    def create_bug_from_commit(self, options, args, tool):
> +	   commit_ids = tool.scm().commit_ids_from_range_arguments(args,
cherry_pick=True)

This won't be very friendly to svn users if they happen to given an extra
argument.  Maybe it should check that it is git and if not, error out with a
friendly message.


> diff --git a/WebKitTools/Scripts/modules/bugzilla.py
b/WebKitTools/Scripts/modules/bugzilla.py
> +    def check_create_bug_response_returning_bug_id_on_success(self,
response_html):

If you don't expect external callers, then prefix it with a _

It is quite long. What about _get_create_bug_id(self, create_bug_response):
or something else (even) shorter?

> +    def create_bug_with_patch(self, bug_title, bug_description, component,
patch_file_object, patch_description, cc, mark_for_review=False):
...
> +	   if not component or component not in [item.name for item in
component_items]:
> +	       component = self.prompt_for_component([item.name for item in
component_items])

This is fine.  I like this better:

	component_names = map(lambda item: item.name, component_items)
	if not component or component not in component_names:
	    component = self.prompt_for_component(component_names)

You choose.