bzr-builder

Merge lp://staging/~spiv/bzr-builder/refactor into lp://staging/~james-w/bzr-builder/trunk-old

  1. refactor
  2. Merge into trunk-old
Proposed by Andrew Bennetts
Status: Merged
Merge reported by: James Westby
Merged at revision: not available
Proposed branch: lp://staging/~spiv/bzr-builder/refactor
Merge into: lp://staging/~james-w/bzr-builder/trunk-old
Prerequisite: lp://staging/~lifeless/bzr-builder/bug-469874
Diff against target: 274 lines (+82/-49)
2 files modified
recipe.py (+67/-36)
tests/test_recipe.py (+15/-13)
To merge this branch: bzr merge lp://staging/~spiv/bzr-builder/refactor
Reviewer Review Type Date Requested Status
James Westby Approve
Review via email: mp+14978@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This basically a replaces a list of 2-tuples where (*, None), (None, *) and (None, None) all mean different things, and replaces them with objects. This also replaces if/elif/... blocks about those cases with good polymorphism.

The names I gave to the objects and methods can probably be improved a fair bit, and I'm happy to do that if you have any suggestions about what these items should be called.

I made this change as part of developing lp:~spiv/bzr-builder/merge-subdirs-479705

Revision history for this message
James Westby (james-w) wrote :

On Wed Nov 18 03:45:21 UTC 2009 Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr-builder/refactor into
> lp:bzr-builder with lp:~lifeless/bzr-builder/bug-469874 as a prerequisite.
>
> Requested reviews:
> James Westby (james-w)
>
>
> This basically a replaces a list of 2-tuples where (*, None), (None, *) and
> (None, None) all mean different things, and replaces them with objects. This
> also replaces if/elif/... blocks about those cases with good polymorphism.

Thanks, code elegance is always appreciated.

> The names I gave to the objects and methods can probably be improved a fair
> bit, and I'm happy to do that if you have any suggestions about what these
> items should be called.

I think the names are good.

My only concern is with .as_tuple(), as the tuple isn't something that has
a natural definition based on the object, and it may make no sense for
some instructions. However, as these were issues with the original code
and doing this means less test changes are needed then I'm happy to
merge it, thanks.

Thanks,

James

Revision history for this message
James Westby (james-w) :
review: Approve
Revision history for this message
James Westby (james-w) wrote :

Landing this is blocked on reviewing/landing the prerequisite branch.

Thanks,

James

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'recipe.py'
--- recipe.py 2009-11-18 03:45:21 +0000
+++ recipe.py 2009-11-18 03:45:21 +0000
@@ -247,11 +247,12 @@
247 changed_revision_id = br_from.last_revision()247 changed_revision_id = br_from.last_revision()
248 if revision_id != changed_revision_id:248 if revision_id != changed_revision_id:
249 changed = True249 changed = True
250 for index, (child_branch, nest_location) in \250 for index, instruction in enumerate(new_branch.child_branches):
251 enumerate(new_branch.child_branches):251 child_branch = instruction.recipe_branch
252 nest_location = instruction.nest_path
252 if_changed_child = None253 if_changed_child = None
253 if if_changed_from is not None:254 if if_changed_from is not None:
254 if_changed_child = if_changed_from.child_branches[index][0]255 if_changed_child = if_changed_from.child_branches[index].recipe_branch
255 if child_branch is not None:256 if child_branch is not None:
256 child_changed = _resolve_revisions_recurse(child_branch,257 child_changed = _resolve_revisions_recurse(child_branch,
257 substitute_revno,258 substitute_revno,
@@ -327,22 +328,8 @@
327 try:328 try:
328 tree_to, br_to = update_branch(base_branch, tree_to, br_to,329 tree_to, br_to = update_branch(base_branch, tree_to, br_to,
329 to_transport)330 to_transport)
330 for child_branch, nest_location in base_branch.child_branches:331 for instruction in base_branch.child_branches:
331 if child_branch is None:332 instruction.apply(target_path, tree_to, br_to)
332 # it's a command
333 proc = subprocess.Popen(nest_location, cwd=target_path,
334 preexec_fn=subprocess_setup, shell=True,
335 stdin=subprocess.PIPE)
336 proc.communicate()
337 if proc.returncode != 0:
338 raise CommandFailedError(nest_location)
339 elif nest_location is not None:
340 # FIXME: pass possible_transports around
341 build_tree(child_branch,
342 target_path=os.path.join(target_path,
343 nest_location))
344 else:
345 merge_branch(child_branch, tree_to, br_to)
346 finally:333 finally:
347 # Is this ok if tree_to is created by pull_or_branch?334 # Is this ok if tree_to is created by pull_or_branch?
348 if br_to is not None:335 if br_to is not None:
@@ -354,7 +341,9 @@
354341
355def _add_child_branches_to_manifest(child_branches, indent_level):342def _add_child_branches_to_manifest(child_branches, indent_level):
356 manifest = ""343 manifest = ""
357 for child_branch, nest_location in child_branches:344 for instruction in child_branches:
345 child_branch = instruction.recipe_branch
346 nest_location = instruction.nest_path
358 if child_branch is None:347 if child_branch is None:
359 manifest += "%s%s %s\n" % (" " * indent_level, RUN_INSTRUCTION,348 manifest += "%s%s %s\n" % (" " * indent_level, RUN_INSTRUCTION,
360 nest_location)349 nest_location)
@@ -390,6 +379,50 @@
390 return manifest379 return manifest
391380
392381
382class ChildBranch(object):
383 """A child branch in a recipe.
384
385 If the nest path is not None it is the path relative to the recipe branch
386 where the child branch should be placed. If it is None then the child
387 branch should be merged instead of nested.
388 """
389
390 def __init__(self, recipe_branch, nest_path=None):
391 self.recipe_branch = recipe_branch
392 self.nest_path = nest_path
393
394 def apply(self, target_path, tree_to, br_to):
395 raise NotImplementedError(self.apply)
396
397 def as_tuple(self):
398 return (self.recipe_branch, self.nest_path)
399
400
401class CommandInstruction(ChildBranch):
402
403 def apply(self, target_path, tree_to, br_to):
404 # it's a command
405 proc = subprocess.Popen(self.nest_path, cwd=target_path,
406 preexec_fn=subprocess_setup, shell=True, stdin=subprocess.PIPE)
407 proc.communicate()
408 if proc.returncode != 0:
409 raise CommandFailedError(self.nest_path)
410
411
412class MergeInstruction(ChildBranch):
413
414 def apply(self, target_path, tree_to, br_to):
415 merge_branch(self.recipe_branch, tree_to, br_to)
416
417
418class NestInstruction(ChildBranch):
419
420 def apply(self, target_path, tree_to, br_to):
421 # FIXME: pass possible_transports around
422 build_tree(self.recipe_branch,
423 target_path=os.path.join(target_path, self.nest_path))
424
425
393class RecipeBranch(object):426class RecipeBranch(object):
394 """A nested structure that represents a Recipe.427 """A nested structure that represents a Recipe.
395428
@@ -397,11 +430,7 @@
397 root branch), and optionally child branches that are either merged430 root branch), and optionally child branches that are either merged
398 or nested.431 or nested.
399432
400 The child_branches attribute is a list of tuples of (RecipeBranch,433 The child_branches attribute is a list of tuples of ChildBranch objects.
401 relative path), where if the relative branch is not None it is the
402 path relative to this branch where the child branch should be placed.
403 If it is None then the child branch should be merged instead.
404
405 The revid attribute records the revid that the url and revspec resolved434 The revid attribute records the revid that the url and revspec resolved
406 to when the RecipeBranch was built, or None if it has not been built.435 to when the RecipeBranch was built, or None if it has not been built.
407 """436 """
@@ -425,7 +454,7 @@
425454
426 :param branch: the RecipeBranch to merge.455 :param branch: the RecipeBranch to merge.
427 """456 """
428 self.child_branches.append((branch, None))457 self.child_branches.append(MergeInstruction(branch))
429458
430 def nest_branch(self, location, branch):459 def nest_branch(self, location, branch):
431 """Nest a child branch in to this one.460 """Nest a child branch in to this one.
@@ -433,16 +462,16 @@
433 :param location: the relative path at which this branch should be nested.462 :param location: the relative path at which this branch should be nested.
434 :param branch: the RecipeBranch to nest.463 :param branch: the RecipeBranch to nest.
435 """464 """
436 assert location not in [b[1] for b in self.child_branches],\465 assert location not in [b.nest_path for b in self.child_branches],\
437 "%s already has branch nested there" % location466 "%s already has branch nested there" % location
438 self.child_branches.append((branch, location))467 self.child_branches.append(NestInstruction(branch, location))
439468
440 def run_command(self, command):469 def run_command(self, command):
441 """Set a command to be run.470 """Set a command to be run.
442471
443 :param command: the command to be run472 :param command: the command to be run
444 """473 """
445 self.child_branches.append((None, command))474 self.child_branches.append(CommandInstruction(None, command))
446475
447 def different_shape_to(self, other_branch):476 def different_shape_to(self, other_branch):
448 """Tests whether the name, url and child_branches are the same"""477 """Tests whether the name, url and child_branches are the same"""
@@ -452,16 +481,18 @@
452 return True481 return True
453 if len(self.child_branches) != len(other_branch.child_branches):482 if len(self.child_branches) != len(other_branch.child_branches):
454 return True483 return True
455 for index, (child_branch, nest_location) in \484 for index, instruction in enumerate(self.child_branches):
456 enumerate(self.child_branches):485 child_branch = instruction.recipe_branch
457 other_child, other_nest_location = \486 nest_location = instruction.nest_path
458 other_branch.child_branches[index]487 other_instruction = other_branch.child_branches[index]
488 other_child_branch = other_instruction.recipe_branch
489 other_nest_location = other_instruction.nest_path
459 if nest_location != other_nest_location:490 if nest_location != other_nest_location:
460 return True491 return True
461 if ((child_branch is None and other_child is not None)492 if ((child_branch is None and other_child_branch is not None)
462 or (child_branch is not None and other_child is None)):493 or (child_branch is not None and other_child_branch is None)):
463 return True494 return True
464 if child_branch.different_shape_to(other_child):495 if child_branch.different_shape_to(other_child_branch):
465 return True496 return True
466 return False497 return False
467498
468499
=== modified file 'tests/test_recipe.py'
--- tests/test_recipe.py 2009-11-18 03:45:21 +0000
+++ tests/test_recipe.py 2009-11-18 03:45:21 +0000
@@ -216,7 +216,7 @@
216 + "merge bar http://bar.org")216 + "merge bar http://bar.org")
217 self.check_base_recipe_branch(base_branch, "http://foo.org/",217 self.check_base_recipe_branch(base_branch, "http://foo.org/",
218 num_child_branches=1)218 num_child_branches=1)
219 child_branch, location = base_branch.child_branches[0]219 child_branch, location = base_branch.child_branches[0].as_tuple()
220 self.assertEqual(None, location)220 self.assertEqual(None, location)
221 self.check_recipe_branch(child_branch, "bar", "http://bar.org")221 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
222222
@@ -225,7 +225,7 @@
225 + "nest bar http://bar.org baz")225 + "nest bar http://bar.org baz")
226 self.check_base_recipe_branch(base_branch, "http://foo.org/",226 self.check_base_recipe_branch(base_branch, "http://foo.org/",
227 num_child_branches=1)227 num_child_branches=1)
228 child_branch, location = base_branch.child_branches[0]228 child_branch, location = base_branch.child_branches[0].as_tuple()
229 self.assertEqual("baz", location)229 self.assertEqual("baz", location)
230 self.check_recipe_branch(child_branch, "bar", "http://bar.org")230 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
231231
@@ -234,10 +234,10 @@
234 + "nest bar http://bar.org baz\nmerge zam lp:zam")234 + "nest bar http://bar.org baz\nmerge zam lp:zam")
235 self.check_base_recipe_branch(base_branch, "http://foo.org/",235 self.check_base_recipe_branch(base_branch, "http://foo.org/",
236 num_child_branches=2)236 num_child_branches=2)
237 child_branch, location = base_branch.child_branches[0]237 child_branch, location = base_branch.child_branches[0].as_tuple()
238 self.assertEqual("baz", location)238 self.assertEqual("baz", location)
239 self.check_recipe_branch(child_branch, "bar", "http://bar.org")239 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
240 child_branch, location = base_branch.child_branches[1]240 child_branch, location = base_branch.child_branches[1].as_tuple()
241 self.assertEqual(None, location)241 self.assertEqual(None, location)
242 self.check_recipe_branch(child_branch, "zam", "lp:zam")242 self.check_recipe_branch(child_branch, "zam", "lp:zam")
243243
@@ -246,10 +246,10 @@
246 + "merge zam lp:zam\nnest bar http://bar.org baz")246 + "merge zam lp:zam\nnest bar http://bar.org baz")
247 self.check_base_recipe_branch(base_branch, "http://foo.org/",247 self.check_base_recipe_branch(base_branch, "http://foo.org/",
248 num_child_branches=2)248 num_child_branches=2)
249 child_branch, location = base_branch.child_branches[0]249 child_branch, location = base_branch.child_branches[0].as_tuple()
250 self.assertEqual(None, location)250 self.assertEqual(None, location)
251 self.check_recipe_branch(child_branch, "zam", "lp:zam")251 self.check_recipe_branch(child_branch, "zam", "lp:zam")
252 child_branch, location = base_branch.child_branches[1]252 child_branch, location = base_branch.child_branches[1].as_tuple()
253 self.assertEqual("baz", location)253 self.assertEqual("baz", location)
254 self.check_recipe_branch(child_branch, "bar", "http://bar.org")254 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
255255
@@ -258,11 +258,11 @@
258 + "nest bar http://bar.org baz\n merge zam lp:zam")258 + "nest bar http://bar.org baz\n merge zam lp:zam")
259 self.check_base_recipe_branch(base_branch, "http://foo.org/",259 self.check_base_recipe_branch(base_branch, "http://foo.org/",
260 num_child_branches=1)260 num_child_branches=1)
261 child_branch, location = base_branch.child_branches[0]261 child_branch, location = base_branch.child_branches[0].as_tuple()
262 self.assertEqual("baz", location)262 self.assertEqual("baz", location)
263 self.check_recipe_branch(child_branch, "bar", "http://bar.org",263 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
264 num_child_branches=1)264 num_child_branches=1)
265 child_branch, location = child_branch.child_branches[0]265 child_branch, location = child_branch.child_branches[0].as_tuple()
266 self.assertEqual(None, location)266 self.assertEqual(None, location)
267 self.check_recipe_branch(child_branch, "zam", "lp:zam")267 self.check_recipe_branch(child_branch, "zam", "lp:zam")
268268
@@ -271,11 +271,11 @@
271 + "nest bar http://bar.org baz\n nest zam lp:zam zoo")271 + "nest bar http://bar.org baz\n nest zam lp:zam zoo")
272 self.check_base_recipe_branch(base_branch, "http://foo.org/",272 self.check_base_recipe_branch(base_branch, "http://foo.org/",
273 num_child_branches=1)273 num_child_branches=1)
274 child_branch, location = base_branch.child_branches[0]274 child_branch, location = base_branch.child_branches[0].as_tuple()
275 self.assertEqual("baz", location)275 self.assertEqual("baz", location)
276 self.check_recipe_branch(child_branch, "bar", "http://bar.org",276 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
277 num_child_branches=1)277 num_child_branches=1)
278 child_branch, location = child_branch.child_branches[0]278 child_branch, location = child_branch.child_branches[0].as_tuple()
279 self.assertEqual("zoo", location)279 self.assertEqual("zoo", location)
280 self.check_recipe_branch(child_branch, "zam", "lp:zam")280 self.check_recipe_branch(child_branch, "zam", "lp:zam")
281281
@@ -286,11 +286,13 @@
286 + "merge zam lp:zam 2")286 + "merge zam lp:zam 2")
287 self.check_base_recipe_branch(base_branch, "http://foo.org/",287 self.check_base_recipe_branch(base_branch, "http://foo.org/",
288 num_child_branches=2, revspec="revid:a")288 num_child_branches=2, revspec="revid:a")
289 child_branch, location = base_branch.child_branches[0]289 instruction = base_branch.child_branches[0]
290 child_branch = instruction.recipe_branch
291 location = instruction.nest_path
290 self.assertEqual("baz", location)292 self.assertEqual("baz", location)
291 self.check_recipe_branch(child_branch, "bar", "http://bar.org",293 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
292 revspec="tag:b")294 revspec="tag:b")
293 child_branch, location = base_branch.child_branches[1]295 child_branch, location = base_branch.child_branches[1].as_tuple()
294 self.assertEqual(None, location)296 self.assertEqual(None, location)
295 self.check_recipe_branch(child_branch, "zam", "lp:zam", revspec="2")297 self.check_recipe_branch(child_branch, "zam", "lp:zam", revspec="2")
296298
@@ -300,7 +302,7 @@
300 + "run touch test \n")302 + "run touch test \n")
301 self.check_base_recipe_branch(base_branch, "http://foo.org/",303 self.check_base_recipe_branch(base_branch, "http://foo.org/",
302 num_child_branches=1)304 num_child_branches=1)
303 child_branch, command = base_branch.child_branches[0]305 child_branch, command = base_branch.child_branches[0].as_tuple()
304 self.assertEqual(None, child_branch)306 self.assertEqual(None, child_branch)
305 self.assertEqual("touch test", command)307 self.assertEqual("touch test", command)
306308

Subscribers

People subscribed via source and target branches