Code review comment for lp://staging/~thumper/pqm/queue-abstraction-2

On Tuesday 05 August 2008 19:12:19 Daniel Watkins wrote:
> Hi Tim,
>
> This is shaping up nicely. I just have a few suggestions which might
> improve it.

Thanks for looking.

> On Tue, 05 Aug 2008 06:34:31 -0000
> Tim Penhey <email address hidden> wrote:
> > === modified file 'bin/pqm'
> > @@ -371,12 +234,16 @@
> > pqm.used_transactions[line[0:-1]] = 1
> >
> > if options.read_mode:
> > - do_read_mode(logger, options)
> > + do_read_mode(logger, options, manager.mail_reply,
> > manager.mail_server,
> > + manager.from_address, manager.nice_from_address)
> It feels to me as if do_read_mode shouldn't exist in bin/pqm any more.
> I'm not entirely sure where it should move to, but perhaps EmailQueue
> would be appropriate?

Hmm.. I was trying hard not to do too much at once :)

Perhaps moving it to EmailQueue would be the best place for this.

> > === added file 'pqm/core.py'
> > @@ -0,0 +1,229 @@
> > +def get_script_queue(queuedir, logger, branch_spec_handler, configp,
> > + options):
> > + """Determine the type of queue from the config."""
> > + # Tim Penhey, 2008-05-29
> > + # Right now there is only one queue type, but this will change
> > RSN.
> > + return EmailQueue(
> > + queuedir, logger, branch_spec_handler, configp, options)
> What other sorts of queue do you envisage there being?

Well, at least one for Launchpad.

> > === modified file 'pqm/script.py'
> It seems to me that the Commands should be moved to their own module,
> as several different scripts may wish to use them...

Again, I was trying to keep the changes small enough. I'm quite happy for
thinks to be broken up more later, but there were challenges enough with
the mass move that was in this branch.

> > === modified file 'pqm/ui/twistd.py'
> > class QueueResource(resource.Resource):
> QueueResource doesn't seem to have been updated to use the new
> abstraction. For example, it directly checks whether 'stop.patch'
> exists, when there is a method on Queue it should be using instead.

OK, I'll look into this later today.

> Hope this helps,

It does, thanks.

Tim