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

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

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

[James Westby (james-w)]

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

[James Westby (james-w)]

review: Approve

[James Westby (james-w)]

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

Thanks,

James

=== 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 @@
                 changed_revision_id = br_from.last_revision()
             if revision_id != changed_revision_id:
                 changed = True
-        for index, (child_branch, nest_location) in \
-            enumerate(new_branch.child_branches):
+        for index, instruction in enumerate(new_branch.child_branches):
+            child_branch = instruction.recipe_branch
+            nest_location = instruction.nest_path
             if_changed_child = None
             if if_changed_from is not None:
-                if_changed_child = if_changed_from.child_branches[index][0]
+                if_changed_child = if_changed_from.child_branches[index].recipe_branch
             if child_branch is not None:
                 child_changed = _resolve_revisions_recurse(child_branch,
                         substitute_revno,
@@ -327,22 +328,8 @@
         try:
             tree_to, br_to = update_branch(base_branch, tree_to, br_to,
                     to_transport)
-            for child_branch, nest_location in base_branch.child_branches:
-                if child_branch is None:
-                    # it's a command
-                    proc = subprocess.Popen(nest_location, cwd=target_path,
-                            preexec_fn=subprocess_setup, shell=True,
-                            stdin=subprocess.PIPE)
-                    proc.communicate()
-                    if proc.returncode != 0:
-                        raise CommandFailedError(nest_location)
-                elif nest_location is not None:
-                    # FIXME: pass possible_transports around
-                    build_tree(child_branch,
-                            target_path=os.path.join(target_path,
-                                nest_location))
-                else:
-                    merge_branch(child_branch, tree_to, br_to)
+            for instruction in base_branch.child_branches:
+                instruction.apply(target_path, tree_to, br_to)
         finally:
             # Is this ok if tree_to is created by pull_or_branch?
             if br_to is not None:
@@ -354,7 +341,9 @@
 
 def _add_child_branches_to_manifest(child_branches, indent_level):
     manifest = ""
-    for child_branch, nest_location in child_branches:
+    for instruction in child_branches:
+        child_branch = instruction.recipe_branch
+        nest_location = instruction.nest_path
         if child_branch is None:
             manifest += "%s%s %s\n" % ("  " * indent_level, RUN_INSTRUCTION,
                     nest_location)
@@ -390,6 +379,50 @@
     return manifest
 
 
+class ChildBranch(object):
+    """A child branch in a recipe.
+    
+    If the nest path is not None it is the path relative to the recipe branch
+    where the child branch should be placed.  If it is None then the child
+    branch should be merged instead of nested.
+    """
+
+    def __init__(self, recipe_branch, nest_path=None):
+        self.recipe_branch = recipe_branch
+        self.nest_path = nest_path
+        
+    def apply(self, target_path, tree_to, br_to):
+        raise NotImplementedError(self.apply)
+
+    def as_tuple(self):
+        return (self.recipe_branch, self.nest_path)
+
+
+class CommandInstruction(ChildBranch):
+
+    def apply(self, target_path, tree_to, br_to):
+        # it's a command
+        proc = subprocess.Popen(self.nest_path, cwd=target_path,
+            preexec_fn=subprocess_setup, shell=True, stdin=subprocess.PIPE)
+        proc.communicate()
+        if proc.returncode != 0:
+            raise CommandFailedError(self.nest_path)
+
+
+class MergeInstruction(ChildBranch):
+
+    def apply(self, target_path, tree_to, br_to):
+        merge_branch(self.recipe_branch, tree_to, br_to)
+
+
+class NestInstruction(ChildBranch):
+
+    def apply(self, target_path, tree_to, br_to):
+        # FIXME: pass possible_transports around
+        build_tree(self.recipe_branch,
+            target_path=os.path.join(target_path, self.nest_path))
+
+
 class RecipeBranch(object):
     """A nested structure that represents a Recipe.
 
@@ -397,11 +430,7 @@
     root branch), and optionally child branches that are either merged
     or nested.
 
-    The child_branches attribute is a list of tuples of (RecipeBranch,
-    relative path), where if the relative branch is not None it is the
-    path relative to this branch where the child branch should be placed.
-    If it is None then the child branch should be merged instead.
-
+    The child_branches attribute is a list of tuples of ChildBranch objects. 
     The revid attribute records the revid that the url and revspec resolved
     to when the RecipeBranch was built, or None if it has not been built.
     """
@@ -425,7 +454,7 @@
 
         :param branch: the RecipeBranch to merge.
         """
-        self.child_branches.append((branch, None))
+        self.child_branches.append(MergeInstruction(branch))
 
     def nest_branch(self, location, branch):
         """Nest a child branch in to this one.
@@ -433,16 +462,16 @@
         :param location: the relative path at which this branch should be nested.
         :param branch: the RecipeBranch to nest.
         """
-        assert location not in [b[1] for b in self.child_branches],\
+        assert location not in [b.nest_path for b in self.child_branches],\
             "%s already has branch nested there" % location
-        self.child_branches.append((branch, location))
+        self.child_branches.append(NestInstruction(branch, location))
 
     def run_command(self, command):
         """Set a command to be run.
 
         :param command: the command to be run
         """
-        self.child_branches.append((None, command))
+        self.child_branches.append(CommandInstruction(None, command))
 
     def different_shape_to(self, other_branch):
         """Tests whether the name, url and child_branches are the same"""
@@ -452,16 +481,18 @@
             return True
         if len(self.child_branches) != len(other_branch.child_branches):
             return True
-        for index, (child_branch, nest_location) in \
-                enumerate(self.child_branches):
-            other_child, other_nest_location = \
-                   other_branch.child_branches[index]
+        for index, instruction in enumerate(self.child_branches):
+            child_branch = instruction.recipe_branch
+            nest_location = instruction.nest_path
+            other_instruction = other_branch.child_branches[index]
+            other_child_branch = other_instruction.recipe_branch
+            other_nest_location = other_instruction.nest_path
             if nest_location != other_nest_location:
                 return True
-            if ((child_branch is None and other_child is not None)
-                    or (child_branch is not None and other_child is None)):
+            if ((child_branch is None and other_child_branch is not None)
+                    or (child_branch is not None and other_child_branch is None)):
                 return True
-            if child_branch.different_shape_to(other_child):
+            if child_branch.different_shape_to(other_child_branch):
                 return True
         return False
 

=== 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 @@
                 + "merge bar http://bar.org")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=1)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual(None, location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org")
 
@@ -225,7 +225,7 @@
                 + "nest bar http://bar.org baz")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=1)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org")
 
@@ -234,10 +234,10 @@
                 + "nest bar http://bar.org baz\nmerge zam lp:zam")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=2)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org")
-        child_branch, location = base_branch.child_branches[1]
+        child_branch, location = base_branch.child_branches[1].as_tuple()
         self.assertEqual(None, location)
         self.check_recipe_branch(child_branch, "zam", "lp:zam")
 
@@ -246,10 +246,10 @@
                 + "merge zam lp:zam\nnest bar http://bar.org baz")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=2)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual(None, location)
         self.check_recipe_branch(child_branch, "zam", "lp:zam")
-        child_branch, location = base_branch.child_branches[1]
+        child_branch, location = base_branch.child_branches[1].as_tuple()
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org")
 
@@ -258,11 +258,11 @@
                 + "nest bar http://bar.org baz\n  merge zam lp:zam")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=1)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org",
                 num_child_branches=1)
-        child_branch, location = child_branch.child_branches[0]
+        child_branch, location = child_branch.child_branches[0].as_tuple()
         self.assertEqual(None, location)
         self.check_recipe_branch(child_branch, "zam", "lp:zam")
 
@@ -271,11 +271,11 @@
                 + "nest bar http://bar.org baz\n  nest zam lp:zam zoo")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=1)
-        child_branch, location = base_branch.child_branches[0]
+        child_branch, location = base_branch.child_branches[0].as_tuple()
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org",
                 num_child_branches=1)
-        child_branch, location = child_branch.child_branches[0]
+        child_branch, location = child_branch.child_branches[0].as_tuple()
         self.assertEqual("zoo", location)
         self.check_recipe_branch(child_branch, "zam", "lp:zam")
 
@@ -286,11 +286,13 @@
                 + "merge zam lp:zam 2")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=2, revspec="revid:a")
-        child_branch, location = base_branch.child_branches[0]
+        instruction = base_branch.child_branches[0]
+        child_branch = instruction.recipe_branch
+        location = instruction.nest_path
         self.assertEqual("baz", location)
         self.check_recipe_branch(child_branch, "bar", "http://bar.org",
                 revspec="tag:b")
-        child_branch, location = base_branch.child_branches[1]
+        child_branch, location = base_branch.child_branches[1].as_tuple()
         self.assertEqual(None, location)
         self.check_recipe_branch(child_branch, "zam", "lp:zam", revspec="2")
 
@@ -300,7 +302,7 @@
                 + "run touch test \n")
         self.check_base_recipe_branch(base_branch, "http://foo.org/",
                 num_child_branches=1)
-        child_branch, command = base_branch.child_branches[0]
+        child_branch, command = base_branch.child_branches[0].as_tuple()
         self.assertEqual(None, child_branch)
         self.assertEqual("touch test", command)