[webkit-reviews] review granted: [Bug 46141] Add a feeder queue that polls bugs.webkit.org for the commit-cluster : [Attachment 68171] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 46141: Add a feeder queue that polls bugs.webkit.org for the commit-cluster
https://bugs.webkit.org/show_bug.cgi?id=46141

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68171&action=review

In general looks great!  Please address the nits above.

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:35
> +	   self.tool = tool

Why not _tool? :)

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:55
> +	   self.update_work_items([patch.id() for patch in patches])

I might have put this out into a patch_ids local (I know it wasn't before). 
Doesn't really matter.

> WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:60
> +	   all_patches =
sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=Tru
e) for bug_id in bug_ids], [])

I might have split this out into a helper function to make this line shorter
were I to write this again.  (Again, I know this is my fault, just noticing as
I re-read the code):

[self._cq_patches_for_bug(bug_id) for bug_id in bug_ids]?  donno.  Again,
doesn't really matter.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:149
> +class FeederQueue(AbstractQueue):

I think you should add a comment above which explains why this makes sense as
an AbstractQueue.  It's not immediately obvious, given that it's such a strange
queue.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:160
> +	   self.feeders = [
> +	       CommitQueueFeeder(self.tool),
> +	   ]

This is nice.  I wonder if it should be a list of classes instead of instances.
 I seem to remember having the same debate with myself for Commands.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:163
> +	   return "synthetic-work-item"

I would have added a comment here.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:166
> +	   return True

Seems this should just be the default.	Silly to make subclasses implement
this.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:169
> +	   self._update()

_update_checkout() maybe?

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:173
> +	   for feeder in self.feeders:
> +	       feeder.feed()
> +	   time.sleep(self._sleep_duration)
> +	   return True

So gorgeous. :)

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:176
> +    def work_item_log_path(self, work_item):
> +	   return os.path.join(self._log_directory(), "feeder.log")

This is a strange case.  We'll end up loggin the same thing twice.  Maybe this
should instead understand returning None and then not opening a work log?

> WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:113
> +	   queue = FeederQueue()
> +	   queue._sleep_duration = 0

This should probably be a FeederQueue subclass since many tests are going to
want this same behavior, no?

> WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:197
> +	       "next_work_item": "",

Are these empty ones really required?  I can't remember.

> WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py:104
> +	   if isinstance(queue, StepSequenceErrorHandler):

Seems we should add a comment about future refactoring here.