Code review comment for lp://staging/~xaav/bzr/tarball-generator

On 3 June 2011 13:40, Geoff <email address hidden> wrote:
> On 3 June 2011 10:41, Geoff <email address hidden> wrote:
>> [fix] I see all the actual 'exporters', and also 'export' itself now return generators which must be consumed for them to work.  Generally if we change an api in such a way that existing callers will be wrong (rather than getting an error), we ought to rename the api too, and of course add a news entry.  Another option would be to keep the existing function names doing the whole job, and add new ones that are generators.
>>
>> I changed how this was done, because I didn't think the code in the finally statement would be executed properly. Please re-review this.
>>
>> It looks like you did change export() back into being a non-generator
>> function, but the others still are.  I am concerned there will be
>> functions that count on the current behaviour so we ought to fix this.
>
> The way the code base is currently set up, there is no need to use the other functions. They should just be passing in the information to the export() function.

If you're reasoning that therefore nobody actually will be using them,
you're probably right, but I wouldn't bet on it. Renaming them is
cheap.

Thanks