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
1=== modified file 'NEWS'
2--- NEWS 2010-04-08 08:48:59 +0000
3+++ NEWS 2010-04-09 04:10:34 +0000
4@@ -57,6 +57,12 @@
5 Internals
6 *********
7
8+* ``bzrlib.commands.Command.run_direct`` is no longer needed - the pre
9+ 2.1 method of calling run() to perform testing or direct use via the API
10+ is now possible again. As part of this, the _operation attribute on
11+ Command is now transient and only exists for the duration of ``run()``.
12+ (Robert Collins)
13+
14 Testing
15 *******
16
17
18=== modified file 'bzrlib/commands.py'
19--- bzrlib/commands.py 2010-04-01 03:52:41 +0000
20+++ bzrlib/commands.py 2010-04-09 04:10:34 +0000
21@@ -411,7 +411,7 @@
22 warn("No help message set for %r" % self)
23 # List of standard options directly supported
24 self.supported_std_options = []
25- self._operation = cleanup.OperationWithCleanups(self.run)
26+ self._setup_run()
27
28 def add_cleanup(self, cleanup_func, *args, **kwargs):
29 """Register a function to call after self.run returns or raises.
30@@ -429,7 +429,9 @@
31
32 This is useful for releasing expensive or contentious resources (such
33 as write locks) before doing further work that does not require those
34- resources (such as writing results to self.outf).
35+ resources (such as writing results to self.outf). Note though, that
36+ as it releases all resources, this may release locks that the command
37+ wants to hold, so use should be done with care.
38 """
39 self._operation.cleanup_now()
40
41@@ -680,11 +682,30 @@
42
43 self._setup_outf()
44
45- return self.run_direct(**all_cmd_args)
46-
47+ return self.run(**all_cmd_args)
48+
49+ def _setup_run(self):
50+ """Wrap the defined run method on self with a cleanup.
51+
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.
54+
55+ If a different form of cleanups are in use by your Command subclass,
56+ you can override this method.
57+ """
58+ class_run = self.run
59+ def run(*args, **kwargs):
60+ self._operation = cleanup.OperationWithCleanups(class_run)
61+ try:
62+ return self._operation.run_simple(*args, **kwargs)
63+ finally:
64+ del self._operation
65+ self.run = run
66+
67+ @deprecated_method(deprecated_in((2, 2, 0)))
68 def run_direct(self, *args, **kwargs):
69- """Call run directly with objects (without parsing an argv list)."""
70- return self._operation.run_simple(*args, **kwargs)
71+ """Deprecated thunk from bzrlib 2.1."""
72+ return self.run(*args, **kwargs)
73
74 def run(self):
75 """Actually run the command.
76@@ -695,6 +716,17 @@
77 Return 0 or None if the command was successful, or a non-zero
78 shell error code if not. It's OK for this method to allow
79 an exception to raise up.
80+
81+ This method is automatically wrapped by Command.__init__ with a
82+ cleanup operation, stored as self._operation. This can be used
83+ via self.add_cleanup to perform automatic cleanups at the end of
84+ run().
85+
86+ The argument for run are assembled by introspection. So for instance,
87+ if your command takes an argument files, you would declare::
88+
89+ def run(self, files=None):
90+ pass
91 """
92 raise NotImplementedError('no implementation of command %r'
93 % self.name())
94
95=== modified file 'bzrlib/tests/commands/test_branch.py'
96--- bzrlib/tests/commands/test_branch.py 2010-02-17 17:11:16 +0000
97+++ bzrlib/tests/commands/test_branch.py 2010-04-09 04:10:34 +0000
98@@ -28,16 +28,16 @@
99
100 def test_branch_remote_local(self):
101 cmd = cmd_branch()
102- cmd.run_direct(self.get_url('branch'), 'local')
103+ cmd.run(self.get_url('branch'), 'local')
104 self.assertEquals(1, len(self.connections))
105
106 def test_branch_local_remote(self):
107 cmd = cmd_branch()
108- cmd.run_direct('branch', self.get_url('remote'))
109+ cmd.run('branch', self.get_url('remote'))
110 self.assertEquals(1, len(self.connections))
111
112 def test_branch_remote_remote(self):
113 cmd = cmd_branch()
114- cmd.run_direct(self.get_url('branch'), self.get_url('remote'))
115+ cmd.run(self.get_url('branch'), self.get_url('remote'))
116 self.assertEquals(2, len(self.connections))
117
118
119=== modified file 'bzrlib/tests/commands/test_cat.py'
120--- bzrlib/tests/commands/test_cat.py 2010-02-23 07:43:11 +0000
121+++ bzrlib/tests/commands/test_cat.py 2010-04-09 04:10:34 +0000
122@@ -44,7 +44,7 @@
123 self.start_logging_connections()
124
125 cmd = cmd_cat()
126- cmd.run_direct(self.get_url('branch/foo'))
127+ cmd.run(self.get_url('branch/foo'))
128 self.assertEquals(1, len(self.connections))
129 self.assertEquals('foo', self.outf.getvalue())
130
131
132=== modified file 'bzrlib/tests/commands/test_checkout.py'
133--- bzrlib/tests/commands/test_checkout.py 2010-02-17 17:11:16 +0000
134+++ bzrlib/tests/commands/test_checkout.py 2010-04-09 04:10:34 +0000
135@@ -27,7 +27,7 @@
136 self.start_logging_connections()
137
138 cmd = cmd_checkout()
139- cmd.run_direct(self.get_url('branch1'), 'local')
140+ cmd.run(self.get_url('branch1'), 'local')
141 self.assertEquals(1, len(self.connections))
142
143 def test_checkout_lightweight(self):
144@@ -36,6 +36,6 @@
145 self.start_logging_connections()
146
147 cmd = cmd_checkout()
148- cmd.run_direct(self.get_url('branch1'), 'local', lightweight=True)
149+ cmd.run(self.get_url('branch1'), 'local', lightweight=True)
150 self.assertEquals(1, len(self.connections))
151
152
153=== modified file 'bzrlib/tests/commands/test_commit.py'
154--- bzrlib/tests/commands/test_commit.py 2010-03-01 19:53:13 +0000
155+++ bzrlib/tests/commands/test_commit.py 2010-04-09 04:10:34 +0000
156@@ -42,7 +42,7 @@
157 # commit do not provide a directory parameter, we have to change dir
158 # manually
159 os.chdir('local')
160- commit.run_direct(message=u'empty commit', unchanged=True)
161+ commit.run(message=u'empty commit', unchanged=True)
162 self.assertEquals(1, len(self.connections))
163
164 def test_commit_both_modified(self):
165
166=== modified file 'bzrlib/tests/commands/test_init.py'
167--- bzrlib/tests/commands/test_init.py 2010-02-17 17:11:16 +0000
168+++ bzrlib/tests/commands/test_init.py 2010-04-09 04:10:34 +0000
169@@ -30,6 +30,6 @@
170 cmd = cmd_init()
171 # We don't care about the ouput but 'outf' should be defined
172 cmd.outf = tests.StringIOWrapper()
173- cmd.run_direct(self.get_url())
174+ cmd.run(self.get_url())
175 self.assertEquals(1, len(self.connections))
176
177
178=== modified file 'bzrlib/tests/commands/test_init_repository.py'
179--- bzrlib/tests/commands/test_init_repository.py 2010-02-17 17:11:16 +0000
180+++ bzrlib/tests/commands/test_init_repository.py 2010-04-09 04:10:34 +0000
181@@ -30,6 +30,6 @@
182 cmd = cmd_init_repository()
183 # We don't care about the ouput but 'outf' should be defined
184 cmd.outf = tests.StringIOWrapper()
185- cmd.run_direct(self.get_url())
186+ cmd.run(self.get_url())
187 self.assertEquals(1, len(self.connections))
188
189
190=== modified file 'bzrlib/tests/commands/test_merge.py'
191--- bzrlib/tests/commands/test_merge.py 2010-02-17 17:11:16 +0000
192+++ bzrlib/tests/commands/test_merge.py 2010-04-09 04:10:34 +0000
193@@ -34,6 +34,5 @@
194 cmd = cmd_merge()
195 # We don't care about the ouput but 'outf' should be defined
196 cmd.outf = StringIOWrapper()
197- cmd.run_direct(self.get_url('branch1'), directory='branch2')
198+ cmd.run(self.get_url('branch1'), directory='branch2')
199 self.assertEquals(1, len(self.connections))
200-
201
202=== modified file 'bzrlib/tests/commands/test_missing.py'
203--- bzrlib/tests/commands/test_missing.py 2010-02-17 17:11:16 +0000
204+++ bzrlib/tests/commands/test_missing.py 2010-04-09 04:10:34 +0000
205@@ -33,6 +33,6 @@
206 cmd = cmd_missing()
207 # We don't care about the ouput but 'outf' should be defined
208 cmd.outf = self.make_utf8_encoded_stringio()
209- cmd.run_direct(self.get_url('branch2'))
210+ cmd.run(self.get_url('branch2'))
211 self.assertEquals(1, len(self.connections))
212
213
214=== modified file 'bzrlib/tests/commands/test_pull.py'
215--- bzrlib/tests/commands/test_pull.py 2010-02-17 17:11:16 +0000
216+++ bzrlib/tests/commands/test_pull.py 2010-04-09 04:10:34 +0000
217@@ -35,7 +35,7 @@
218 cmd = builtins.cmd_pull()
219 # We don't care about the ouput but 'outf' should be defined
220 cmd.outf = tests.StringIOWrapper()
221- cmd.run_direct(self.get_url('branch1'), directory='branch2')
222+ cmd.run(self.get_url('branch1'), directory='branch2')
223 self.assertEquals(1, len(self.connections))
224
225 def test_pull_with_bound_branch(self):
226@@ -53,6 +53,6 @@
227 pull = builtins.cmd_pull()
228 # We don't care about the ouput but 'outf' should be defined
229 pull.outf = tests.StringIOWrapper()
230- pull.run_direct(self.get_url('remote'), directory='local')
231+ pull.run(self.get_url('remote'), directory='local')
232 self.assertEquals(1, len(self.connections))
233
234
235=== modified file 'bzrlib/tests/commands/test_push.py'
236--- bzrlib/tests/commands/test_push.py 2010-02-17 17:11:16 +0000
237+++ bzrlib/tests/commands/test_push.py 2010-04-09 04:10:34 +0000
238@@ -30,7 +30,7 @@
239 cmd = cmd_push()
240 # We don't care about the ouput but 'outf' should be defined
241 cmd.outf = tests.StringIOWrapper()
242- cmd.run_direct(self.get_url('remote'), directory='branch')
243+ cmd.run(self.get_url('remote'), directory='branch')
244 self.assertEquals(1, len(self.connections))
245
246 def test_push_onto_stacked(self):
247@@ -41,6 +41,6 @@
248
249 cmd = cmd_push()
250 cmd.outf = tests.StringIOWrapper()
251- cmd.run_direct(self.get_url('remote'), directory='source',
252+ cmd.run(self.get_url('remote'), directory='source',
253 stacked_on=self.get_url('base'))
254 self.assertEqual(1, len(self.connections))
255
256=== modified file 'bzrlib/tests/commands/test_update.py'
257--- bzrlib/tests/commands/test_update.py 2010-02-17 17:11:16 +0000
258+++ bzrlib/tests/commands/test_update.py 2010-04-09 04:10:34 +0000
259@@ -40,6 +40,6 @@
260 # update needs the encoding from outf to print URLs
261 update.outf = tests.StringIOWrapper()
262 # update calls it 'dir' where other commands calls it 'directory'
263- update.run_direct(dir='local')
264+ update.run(dir='local')
265 self.assertEquals(1, len(self.connections))
266