LGTM On Tue, May 11, 2010 at 7:47 AM, Michael Hanselmann <han...@google.com>wrote:
> Hi David > > Unless noted otherwise, changes have been made in the interdiff at the end. > > 2010/5/10 David Knowles <dknow...@google.com>: > > On Mon, May 10, 2010 at 11:30 AM, Michael Hanselmann <han...@google.com> > > wrote: > >> --- /dev/null > >> +++ b/lib/rapi/client_utils.py > >> +class RapiJobPollCb(cli.JobPollCbBase): > > > > What's "Cb"? Is that a callback? If so, it would be much more clear to > call > > it "callback". > > It's a callback and back when we implemented mcpu.OpExecCbBase, we > decided that short names are better. > > >> + def __init__(self, cl): > >> + """Initializes this class. > >> + > >> + @param cl: RAPI client instance > > > > A more descriptive variable name would be nice. > > “cl” is Ganeti convention for clients. See cli.py. > > >> + cli.JobPollCbBase.__init__(self) > >> + > >> + self.cl = cl > >> + > >> + def WaitForJobChangeOnce(self, job_id, fields, > >> + prev_job_info, prev_log_serial): > >> + """Waits for changes on a job. > > > > This doesn't completely describe what this method does. It waits for a > > change, then what? > > Also, missing a @return doctag. > > “WaitForJobChange[Once]” is, like “SubmitJob” or “CancelJob”, a > forwarded function from the job queue (jqueue.py). Since pydoc doesn't > have support for copying another functions'd docstring, we depend on > the reader to have some context. > > My concern with this and the above is that this is in a client library, and the client reading this code may not have familiarity with underlying Ganeti core code. But I'm not *that* concerned with it. :)
