Code review comment for lp:~lifeless/bzr/commands

On Mon, 2010-04-12 at 04:12 +0000, Martin Pool wrote:
> On 9 April 2010 14:10, Robert Collins <email address hidden> wrote:
> > Robert Collins has proposed merging lp:~lifeless/bzr/commands into lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> >
> >
> > This branch removes(deprecates) the helper function added in 2.1, Command.run_direct. run_direct is confusingly named (it is what was used to run with cleanups), and had the undesirable effect of making a previously safe to call function (command.run) unsafe to call. By decorating run, it is always safe to call run directly, and run stays both public for subclassing and public for calling.
> >
> > Doing this fixes a number of bugs in bzrtools, loom and other plugins where the run method is currently called. We may wish to port this to 2.1 too, but I don't know if its considered 'minimal' enough.
>
> We shouldn't make any deprecations in stable series unless there is a
> really strong reason, eg that it is the only feasible way to fix a
> serious bug. I don't know if the other parts of this would make sense
> or be worth backporting without that change?

I think having people change code to call a new function we've removed
immediately after isn't worth a deprecation-in-stable. However we should
change the docstring to say 'deprecated in 2.2' and have them use run();
and yes I think taking the other changes than the deprecation would be
sensible for 2.1.

-Rob