Bazaar

Merge lp:~lifeless/bzr/commands into lp:bzr

  1. commands
  2. Merge into bzr.dev
Proposed by Robert Collins
Status: Merged
Approved by: Andrew Bennetts
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~lifeless/bzr/commands
Merge into: lp:bzr
Diff against target: 265 lines (+60/-23)
13 files modified
NEWS (+6/-0)
bzrlib/commands.py (+38/-6)
bzrlib/tests/commands/test_branch.py (+3/-3)
bzrlib/tests/commands/test_cat.py (+1/-1)
bzrlib/tests/commands/test_checkout.py (+2/-2)
bzrlib/tests/commands/test_commit.py (+1/-1)
bzrlib/tests/commands/test_init.py (+1/-1)
bzrlib/tests/commands/test_init_repository.py (+1/-1)
bzrlib/tests/commands/test_merge.py (+1/-2)
bzrlib/tests/commands/test_missing.py (+1/-1)
bzrlib/tests/commands/test_pull.py (+2/-2)
bzrlib/tests/commands/test_push.py (+2/-2)
bzrlib/tests/commands/test_update.py (+1/-1)
To merge this branch: bzr merge lp:~lifeless/bzr/commands
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Review via email: mp+23068@code.launchpad.net

Commit message

(robertc) Make Command.run safe to call directly. (Robert Collins)

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

I think I like it, but I'm going to sleep on it over the weekend.

Revision history for this message
Andrew Bennetts (spiv) wrote :

52 + This is called by __init__ to make the Command be able to be run
53 + by just calling run(), as it could be before cleanups were added.

This part of the docstring is a bit unclear. People using the Command API shouldn't need to know about the history of the API to be able to use it.

Perhaps it should say something like this instead:

    This is called by __init__ to make it possible to call cmd.run()
    directly.

I'm still a bit uncomfortable with being so tricky, but it does seem like the best compromise that preserves the APIs we like (that subclasses can override run; callers can invoke run directly; cleanups added during run will be executed).

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

Thanks, I've tweaked the doc string with that insight, and its on the way now.

Revision history for this message
Martin Pool (mbp) 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?

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

Revision history for this message
Robert Collins (lifeless) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-04-08 08:48:59 +0000
+++ NEWS 2010-04-09 04:10:34 +0000
@@ -57,6 +57,12 @@
57Internals57Internals
58*********58*********
5959
60* ``bzrlib.commands.Command.run_direct`` is no longer needed - the pre
61 2.1 method of calling run() to perform testing or direct use via the API
62 is now possible again. As part of this, the _operation attribute on
63 Command is now transient and only exists for the duration of ``run()``.
64 (Robert Collins)
65
60Testing66Testing
61*******67*******
6268
6369
=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py 2010-04-01 03:52:41 +0000
+++ bzrlib/commands.py 2010-04-09 04:10:34 +0000
@@ -411,7 +411,7 @@
411 warn("No help message set for %r" % self)411 warn("No help message set for %r" % self)
412 # List of standard options directly supported412 # List of standard options directly supported
413 self.supported_std_options = []413 self.supported_std_options = []
414 self._operation = cleanup.OperationWithCleanups(self.run)414 self._setup_run()
415415
416 def add_cleanup(self, cleanup_func, *args, **kwargs):416 def add_cleanup(self, cleanup_func, *args, **kwargs):
417 """Register a function to call after self.run returns or raises.417 """Register a function to call after self.run returns or raises.
@@ -429,7 +429,9 @@
429429
430 This is useful for releasing expensive or contentious resources (such430 This is useful for releasing expensive or contentious resources (such
431 as write locks) before doing further work that does not require those431 as write locks) before doing further work that does not require those
432 resources (such as writing results to self.outf).432 resources (such as writing results to self.outf). Note though, that
433 as it releases all resources, this may release locks that the command
434 wants to hold, so use should be done with care.
433 """435 """
434 self._operation.cleanup_now()436 self._operation.cleanup_now()
435437
@@ -680,11 +682,30 @@
680682
681 self._setup_outf()683 self._setup_outf()
682684
683 return self.run_direct(**all_cmd_args)685 return self.run(**all_cmd_args)
684686
687 def _setup_run(self):
688 """Wrap the defined run method on self with a cleanup.
689
690 This is called by __init__ to make the Command be able to be run
691 by just calling run(), as it could be before cleanups were added.
692
693 If a different form of cleanups are in use by your Command subclass,
694 you can override this method.
695 """
696 class_run = self.run
697 def run(*args, **kwargs):
698 self._operation = cleanup.OperationWithCleanups(class_run)
699 try:
700 return self._operation.run_simple(*args, **kwargs)
701 finally:
702 del self._operation
703 self.run = run
704
705 @deprecated_method(deprecated_in((2, 2, 0)))
685 def run_direct(self, *args, **kwargs):706 def run_direct(self, *args, **kwargs):
686 """Call run directly with objects (without parsing an argv list)."""707 """Deprecated thunk from bzrlib 2.1."""
687 return self._operation.run_simple(*args, **kwargs)708 return self.run(*args, **kwargs)
688709
689 def run(self):710 def run(self):
690 """Actually run the command.711 """Actually run the command.
@@ -695,6 +716,17 @@
695 Return 0 or None if the command was successful, or a non-zero716 Return 0 or None if the command was successful, or a non-zero
696 shell error code if not. It's OK for this method to allow717 shell error code if not. It's OK for this method to allow
697 an exception to raise up.718 an exception to raise up.
719
720 This method is automatically wrapped by Command.__init__ with a
721 cleanup operation, stored as self._operation. This can be used
722 via self.add_cleanup to perform automatic cleanups at the end of
723 run().
724
725 The argument for run are assembled by introspection. So for instance,
726 if your command takes an argument files, you would declare::
727
728 def run(self, files=None):
729 pass
698 """730 """
699 raise NotImplementedError('no implementation of command %r'731 raise NotImplementedError('no implementation of command %r'
700 % self.name())732 % self.name())
701733
=== modified file 'bzrlib/tests/commands/test_branch.py'
--- bzrlib/tests/commands/test_branch.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_branch.py 2010-04-09 04:10:34 +0000
@@ -28,16 +28,16 @@
2828
29 def test_branch_remote_local(self):29 def test_branch_remote_local(self):
30 cmd = cmd_branch()30 cmd = cmd_branch()
31 cmd.run_direct(self.get_url('branch'), 'local')31 cmd.run(self.get_url('branch'), 'local')
32 self.assertEquals(1, len(self.connections))32 self.assertEquals(1, len(self.connections))
3333
34 def test_branch_local_remote(self):34 def test_branch_local_remote(self):
35 cmd = cmd_branch()35 cmd = cmd_branch()
36 cmd.run_direct('branch', self.get_url('remote'))36 cmd.run('branch', self.get_url('remote'))
37 self.assertEquals(1, len(self.connections))37 self.assertEquals(1, len(self.connections))
3838
39 def test_branch_remote_remote(self):39 def test_branch_remote_remote(self):
40 cmd = cmd_branch()40 cmd = cmd_branch()
41 cmd.run_direct(self.get_url('branch'), self.get_url('remote'))41 cmd.run(self.get_url('branch'), self.get_url('remote'))
42 self.assertEquals(2, len(self.connections))42 self.assertEquals(2, len(self.connections))
4343
4444
=== modified file 'bzrlib/tests/commands/test_cat.py'
--- bzrlib/tests/commands/test_cat.py 2010-02-23 07:43:11 +0000
+++ bzrlib/tests/commands/test_cat.py 2010-04-09 04:10:34 +0000
@@ -44,7 +44,7 @@
44 self.start_logging_connections()44 self.start_logging_connections()
4545
46 cmd = cmd_cat()46 cmd = cmd_cat()
47 cmd.run_direct(self.get_url('branch/foo'))47 cmd.run(self.get_url('branch/foo'))
48 self.assertEquals(1, len(self.connections))48 self.assertEquals(1, len(self.connections))
49 self.assertEquals('foo', self.outf.getvalue())49 self.assertEquals('foo', self.outf.getvalue())
5050
5151
=== modified file 'bzrlib/tests/commands/test_checkout.py'
--- bzrlib/tests/commands/test_checkout.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_checkout.py 2010-04-09 04:10:34 +0000
@@ -27,7 +27,7 @@
27 self.start_logging_connections()27 self.start_logging_connections()
2828
29 cmd = cmd_checkout()29 cmd = cmd_checkout()
30 cmd.run_direct(self.get_url('branch1'), 'local')30 cmd.run(self.get_url('branch1'), 'local')
31 self.assertEquals(1, len(self.connections))31 self.assertEquals(1, len(self.connections))
3232
33 def test_checkout_lightweight(self):33 def test_checkout_lightweight(self):
@@ -36,6 +36,6 @@
36 self.start_logging_connections()36 self.start_logging_connections()
3737
38 cmd = cmd_checkout()38 cmd = cmd_checkout()
39 cmd.run_direct(self.get_url('branch1'), 'local', lightweight=True)39 cmd.run(self.get_url('branch1'), 'local', lightweight=True)
40 self.assertEquals(1, len(self.connections))40 self.assertEquals(1, len(self.connections))
4141
4242
=== modified file 'bzrlib/tests/commands/test_commit.py'
--- bzrlib/tests/commands/test_commit.py 2010-03-01 19:53:13 +0000
+++ bzrlib/tests/commands/test_commit.py 2010-04-09 04:10:34 +0000
@@ -42,7 +42,7 @@
42 # commit do not provide a directory parameter, we have to change dir42 # commit do not provide a directory parameter, we have to change dir
43 # manually43 # manually
44 os.chdir('local')44 os.chdir('local')
45 commit.run_direct(message=u'empty commit', unchanged=True)45 commit.run(message=u'empty commit', unchanged=True)
46 self.assertEquals(1, len(self.connections))46 self.assertEquals(1, len(self.connections))
4747
48 def test_commit_both_modified(self):48 def test_commit_both_modified(self):
4949
=== modified file 'bzrlib/tests/commands/test_init.py'
--- bzrlib/tests/commands/test_init.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_init.py 2010-04-09 04:10:34 +0000
@@ -30,6 +30,6 @@
30 cmd = cmd_init()30 cmd = cmd_init()
31 # We don't care about the ouput but 'outf' should be defined31 # We don't care about the ouput but 'outf' should be defined
32 cmd.outf = tests.StringIOWrapper()32 cmd.outf = tests.StringIOWrapper()
33 cmd.run_direct(self.get_url())33 cmd.run(self.get_url())
34 self.assertEquals(1, len(self.connections))34 self.assertEquals(1, len(self.connections))
3535
3636
=== modified file 'bzrlib/tests/commands/test_init_repository.py'
--- bzrlib/tests/commands/test_init_repository.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_init_repository.py 2010-04-09 04:10:34 +0000
@@ -30,6 +30,6 @@
30 cmd = cmd_init_repository()30 cmd = cmd_init_repository()
31 # We don't care about the ouput but 'outf' should be defined31 # We don't care about the ouput but 'outf' should be defined
32 cmd.outf = tests.StringIOWrapper()32 cmd.outf = tests.StringIOWrapper()
33 cmd.run_direct(self.get_url())33 cmd.run(self.get_url())
34 self.assertEquals(1, len(self.connections))34 self.assertEquals(1, len(self.connections))
3535
3636
=== modified file 'bzrlib/tests/commands/test_merge.py'
--- bzrlib/tests/commands/test_merge.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_merge.py 2010-04-09 04:10:34 +0000
@@ -34,6 +34,5 @@
34 cmd = cmd_merge()34 cmd = cmd_merge()
35 # We don't care about the ouput but 'outf' should be defined35 # We don't care about the ouput but 'outf' should be defined
36 cmd.outf = StringIOWrapper()36 cmd.outf = StringIOWrapper()
37 cmd.run_direct(self.get_url('branch1'), directory='branch2')37 cmd.run(self.get_url('branch1'), directory='branch2')
38 self.assertEquals(1, len(self.connections))38 self.assertEquals(1, len(self.connections))
39
4039
=== modified file 'bzrlib/tests/commands/test_missing.py'
--- bzrlib/tests/commands/test_missing.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_missing.py 2010-04-09 04:10:34 +0000
@@ -33,6 +33,6 @@
33 cmd = cmd_missing()33 cmd = cmd_missing()
34 # We don't care about the ouput but 'outf' should be defined34 # We don't care about the ouput but 'outf' should be defined
35 cmd.outf = self.make_utf8_encoded_stringio()35 cmd.outf = self.make_utf8_encoded_stringio()
36 cmd.run_direct(self.get_url('branch2'))36 cmd.run(self.get_url('branch2'))
37 self.assertEquals(1, len(self.connections))37 self.assertEquals(1, len(self.connections))
3838
3939
=== modified file 'bzrlib/tests/commands/test_pull.py'
--- bzrlib/tests/commands/test_pull.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_pull.py 2010-04-09 04:10:34 +0000
@@ -35,7 +35,7 @@
35 cmd = builtins.cmd_pull()35 cmd = builtins.cmd_pull()
36 # We don't care about the ouput but 'outf' should be defined36 # We don't care about the ouput but 'outf' should be defined
37 cmd.outf = tests.StringIOWrapper()37 cmd.outf = tests.StringIOWrapper()
38 cmd.run_direct(self.get_url('branch1'), directory='branch2')38 cmd.run(self.get_url('branch1'), directory='branch2')
39 self.assertEquals(1, len(self.connections))39 self.assertEquals(1, len(self.connections))
4040
41 def test_pull_with_bound_branch(self):41 def test_pull_with_bound_branch(self):
@@ -53,6 +53,6 @@
53 pull = builtins.cmd_pull()53 pull = builtins.cmd_pull()
54 # We don't care about the ouput but 'outf' should be defined54 # We don't care about the ouput but 'outf' should be defined
55 pull.outf = tests.StringIOWrapper()55 pull.outf = tests.StringIOWrapper()
56 pull.run_direct(self.get_url('remote'), directory='local')56 pull.run(self.get_url('remote'), directory='local')
57 self.assertEquals(1, len(self.connections))57 self.assertEquals(1, len(self.connections))
5858
5959
=== modified file 'bzrlib/tests/commands/test_push.py'
--- bzrlib/tests/commands/test_push.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_push.py 2010-04-09 04:10:34 +0000
@@ -30,7 +30,7 @@
30 cmd = cmd_push()30 cmd = cmd_push()
31 # We don't care about the ouput but 'outf' should be defined31 # We don't care about the ouput but 'outf' should be defined
32 cmd.outf = tests.StringIOWrapper()32 cmd.outf = tests.StringIOWrapper()
33 cmd.run_direct(self.get_url('remote'), directory='branch')33 cmd.run(self.get_url('remote'), directory='branch')
34 self.assertEquals(1, len(self.connections))34 self.assertEquals(1, len(self.connections))
3535
36 def test_push_onto_stacked(self):36 def test_push_onto_stacked(self):
@@ -41,6 +41,6 @@
4141
42 cmd = cmd_push()42 cmd = cmd_push()
43 cmd.outf = tests.StringIOWrapper()43 cmd.outf = tests.StringIOWrapper()
44 cmd.run_direct(self.get_url('remote'), directory='source',44 cmd.run(self.get_url('remote'), directory='source',
45 stacked_on=self.get_url('base'))45 stacked_on=self.get_url('base'))
46 self.assertEqual(1, len(self.connections))46 self.assertEqual(1, len(self.connections))
4747
=== modified file 'bzrlib/tests/commands/test_update.py'
--- bzrlib/tests/commands/test_update.py 2010-02-17 17:11:16 +0000
+++ bzrlib/tests/commands/test_update.py 2010-04-09 04:10:34 +0000
@@ -40,6 +40,6 @@
40 # update needs the encoding from outf to print URLs40 # update needs the encoding from outf to print URLs
41 update.outf = tests.StringIOWrapper()41 update.outf = tests.StringIOWrapper()
42 # update calls it 'dir' where other commands calls it 'directory'42 # update calls it 'dir' where other commands calls it 'directory'
43 update.run_direct(dir='local')43 update.run(dir='local')
44 self.assertEquals(1, len(self.connections))44 self.assertEquals(1, len(self.connections))
4545