Code review comment for lp://staging/~mbp/bzr/402022-merge-preview

2009/7/24 Aaron Bentley <email address hidden>:
>> This is a minor change to cmd_merge's use of the cleanup list, so that it doesn't break the way qbzr wrapped this command.
>
> On the other hand, it's an API break, and it will break bzr-pipeline.
> Since we've already had an API break, we might as well stick with the
> one we've got.
>
> I'm surprised to see this patch being proposed on the basis of not
> breaking qbzr.  bzr API updates break bzrtools all the time, and I never
> ask for them to be reverted just so that I don't have to update bzrtools.

I don't normally revert things for the sake of qbzr, I just started on
this bug because it was originally reported as a bzr bug, and it
seemed so to me.

If bzr breaks bzrtools and you want to propose a patch to restore an
inadvertently broken api you're welcome to. I think we've taken at
least some of them in the past.

>
>>  I think it's at least somewhat cleaner anyhow, and you could make a case that having a list of cleanups to run when the command completes would help extension in other cases...
>
> I don't really agree, though I might be biased since I wrote those changes.
>
> I don't think it's good practice to initialize instance variables
> outside the constructor.  Even if you initialize them to None and assign
> to them later, they should still be valid names for the lifetime of the
> object

Good point.

> I'm not really comfortable with methods having hidden side-effects, such
> as the creation and mutation of self.cleanups.

Well, 'hidden side effects' sound pretty bad by definition. But if
code is in a method of a stateful object it's reasonable for it to
mutate the object. For me the question is, is it reasonable for that
to be state on the cmd object; at present it almost is. Is that the
main reason you want to pass it?

--
Martin <http://launchpad.net/~mbp/>