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
1=== modified file 'recipe.py'
2--- recipe.py 2009-11-18 03:45:21 +0000
3+++ recipe.py 2009-11-18 03:45:21 +0000
4@@ -247,11 +247,12 @@
5 changed_revision_id = br_from.last_revision()
6 if revision_id != changed_revision_id:
7 changed = True
8- for index, (child_branch, nest_location) in \
9- enumerate(new_branch.child_branches):
10+ for index, instruction in enumerate(new_branch.child_branches):
11+ child_branch = instruction.recipe_branch
12+ nest_location = instruction.nest_path
13 if_changed_child = None
14 if if_changed_from is not None:
15- if_changed_child = if_changed_from.child_branches[index][0]
16+ if_changed_child = if_changed_from.child_branches[index].recipe_branch
17 if child_branch is not None:
18 child_changed = _resolve_revisions_recurse(child_branch,
19 substitute_revno,
20@@ -327,22 +328,8 @@
21 try:
22 tree_to, br_to = update_branch(base_branch, tree_to, br_to,
23 to_transport)
24- for child_branch, nest_location in base_branch.child_branches:
25- if child_branch is None:
26- # it's a command
27- proc = subprocess.Popen(nest_location, cwd=target_path,
28- preexec_fn=subprocess_setup, shell=True,
29- stdin=subprocess.PIPE)
30- proc.communicate()
31- if proc.returncode != 0:
32- raise CommandFailedError(nest_location)
33- elif nest_location is not None:
34- # FIXME: pass possible_transports around
35- build_tree(child_branch,
36- target_path=os.path.join(target_path,
37- nest_location))
38- else:
39- merge_branch(child_branch, tree_to, br_to)
40+ for instruction in base_branch.child_branches:
41+ instruction.apply(target_path, tree_to, br_to)
42 finally:
43 # Is this ok if tree_to is created by pull_or_branch?
44 if br_to is not None:
45@@ -354,7 +341,9 @@
46
47 def _add_child_branches_to_manifest(child_branches, indent_level):
48 manifest = ""
49- for child_branch, nest_location in child_branches:
50+ for instruction in child_branches:
51+ child_branch = instruction.recipe_branch
52+ nest_location = instruction.nest_path
53 if child_branch is None:
54 manifest += "%s%s %s\n" % (" " * indent_level, RUN_INSTRUCTION,
55 nest_location)
56@@ -390,6 +379,50 @@
57 return manifest
58
59
60+class ChildBranch(object):
61+ """A child branch in a recipe.
62+
63+ If the nest path is not None it is the path relative to the recipe branch
64+ where the child branch should be placed. If it is None then the child
65+ branch should be merged instead of nested.
66+ """
67+
68+ def __init__(self, recipe_branch, nest_path=None):
69+ self.recipe_branch = recipe_branch
70+ self.nest_path = nest_path
71+
72+ def apply(self, target_path, tree_to, br_to):
73+ raise NotImplementedError(self.apply)
74+
75+ def as_tuple(self):
76+ return (self.recipe_branch, self.nest_path)
77+
78+
79+class CommandInstruction(ChildBranch):
80+
81+ def apply(self, target_path, tree_to, br_to):
82+ # it's a command
83+ proc = subprocess.Popen(self.nest_path, cwd=target_path,
84+ preexec_fn=subprocess_setup, shell=True, stdin=subprocess.PIPE)
85+ proc.communicate()
86+ if proc.returncode != 0:
87+ raise CommandFailedError(self.nest_path)
88+
89+
90+class MergeInstruction(ChildBranch):
91+
92+ def apply(self, target_path, tree_to, br_to):
93+ merge_branch(self.recipe_branch, tree_to, br_to)
94+
95+
96+class NestInstruction(ChildBranch):
97+
98+ def apply(self, target_path, tree_to, br_to):
99+ # FIXME: pass possible_transports around
100+ build_tree(self.recipe_branch,
101+ target_path=os.path.join(target_path, self.nest_path))
102+
103+
104 class RecipeBranch(object):
105 """A nested structure that represents a Recipe.
106
107@@ -397,11 +430,7 @@
108 root branch), and optionally child branches that are either merged
109 or nested.
110
111- The child_branches attribute is a list of tuples of (RecipeBranch,
112- relative path), where if the relative branch is not None it is the
113- path relative to this branch where the child branch should be placed.
114- If it is None then the child branch should be merged instead.
115-
116+ The child_branches attribute is a list of tuples of ChildBranch objects.
117 The revid attribute records the revid that the url and revspec resolved
118 to when the RecipeBranch was built, or None if it has not been built.
119 """
120@@ -425,7 +454,7 @@
121
122 :param branch: the RecipeBranch to merge.
123 """
124- self.child_branches.append((branch, None))
125+ self.child_branches.append(MergeInstruction(branch))
126
127 def nest_branch(self, location, branch):
128 """Nest a child branch in to this one.
129@@ -433,16 +462,16 @@
130 :param location: the relative path at which this branch should be nested.
131 :param branch: the RecipeBranch to nest.
132 """
133- assert location not in [b[1] for b in self.child_branches],\
134+ assert location not in [b.nest_path for b in self.child_branches],\
135 "%s already has branch nested there" % location
136- self.child_branches.append((branch, location))
137+ self.child_branches.append(NestInstruction(branch, location))
138
139 def run_command(self, command):
140 """Set a command to be run.
141
142 :param command: the command to be run
143 """
144- self.child_branches.append((None, command))
145+ self.child_branches.append(CommandInstruction(None, command))
146
147 def different_shape_to(self, other_branch):
148 """Tests whether the name, url and child_branches are the same"""
149@@ -452,16 +481,18 @@
150 return True
151 if len(self.child_branches) != len(other_branch.child_branches):
152 return True
153- for index, (child_branch, nest_location) in \
154- enumerate(self.child_branches):
155- other_child, other_nest_location = \
156- other_branch.child_branches[index]
157+ for index, instruction in enumerate(self.child_branches):
158+ child_branch = instruction.recipe_branch
159+ nest_location = instruction.nest_path
160+ other_instruction = other_branch.child_branches[index]
161+ other_child_branch = other_instruction.recipe_branch
162+ other_nest_location = other_instruction.nest_path
163 if nest_location != other_nest_location:
164 return True
165- if ((child_branch is None and other_child is not None)
166- or (child_branch is not None and other_child is None)):
167+ if ((child_branch is None and other_child_branch is not None)
168+ or (child_branch is not None and other_child_branch is None)):
169 return True
170- if child_branch.different_shape_to(other_child):
171+ if child_branch.different_shape_to(other_child_branch):
172 return True
173 return False
174
175
176=== modified file 'tests/test_recipe.py'
177--- tests/test_recipe.py 2009-11-18 03:45:21 +0000
178+++ tests/test_recipe.py 2009-11-18 03:45:21 +0000
179@@ -216,7 +216,7 @@
180 + "merge bar http://bar.org")
181 self.check_base_recipe_branch(base_branch, "http://foo.org/",
182 num_child_branches=1)
183- child_branch, location = base_branch.child_branches[0]
184+ child_branch, location = base_branch.child_branches[0].as_tuple()
185 self.assertEqual(None, location)
186 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
187
188@@ -225,7 +225,7 @@
189 + "nest bar http://bar.org baz")
190 self.check_base_recipe_branch(base_branch, "http://foo.org/",
191 num_child_branches=1)
192- child_branch, location = base_branch.child_branches[0]
193+ child_branch, location = base_branch.child_branches[0].as_tuple()
194 self.assertEqual("baz", location)
195 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
196
197@@ -234,10 +234,10 @@
198 + "nest bar http://bar.org baz\nmerge zam lp:zam")
199 self.check_base_recipe_branch(base_branch, "http://foo.org/",
200 num_child_branches=2)
201- child_branch, location = base_branch.child_branches[0]
202+ child_branch, location = base_branch.child_branches[0].as_tuple()
203 self.assertEqual("baz", location)
204 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
205- child_branch, location = base_branch.child_branches[1]
206+ child_branch, location = base_branch.child_branches[1].as_tuple()
207 self.assertEqual(None, location)
208 self.check_recipe_branch(child_branch, "zam", "lp:zam")
209
210@@ -246,10 +246,10 @@
211 + "merge zam lp:zam\nnest bar http://bar.org baz")
212 self.check_base_recipe_branch(base_branch, "http://foo.org/",
213 num_child_branches=2)
214- child_branch, location = base_branch.child_branches[0]
215+ child_branch, location = base_branch.child_branches[0].as_tuple()
216 self.assertEqual(None, location)
217 self.check_recipe_branch(child_branch, "zam", "lp:zam")
218- child_branch, location = base_branch.child_branches[1]
219+ child_branch, location = base_branch.child_branches[1].as_tuple()
220 self.assertEqual("baz", location)
221 self.check_recipe_branch(child_branch, "bar", "http://bar.org")
222
223@@ -258,11 +258,11 @@
224 + "nest bar http://bar.org baz\n merge zam lp:zam")
225 self.check_base_recipe_branch(base_branch, "http://foo.org/",
226 num_child_branches=1)
227- child_branch, location = base_branch.child_branches[0]
228+ child_branch, location = base_branch.child_branches[0].as_tuple()
229 self.assertEqual("baz", location)
230 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
231 num_child_branches=1)
232- child_branch, location = child_branch.child_branches[0]
233+ child_branch, location = child_branch.child_branches[0].as_tuple()
234 self.assertEqual(None, location)
235 self.check_recipe_branch(child_branch, "zam", "lp:zam")
236
237@@ -271,11 +271,11 @@
238 + "nest bar http://bar.org baz\n nest zam lp:zam zoo")
239 self.check_base_recipe_branch(base_branch, "http://foo.org/",
240 num_child_branches=1)
241- child_branch, location = base_branch.child_branches[0]
242+ child_branch, location = base_branch.child_branches[0].as_tuple()
243 self.assertEqual("baz", location)
244 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
245 num_child_branches=1)
246- child_branch, location = child_branch.child_branches[0]
247+ child_branch, location = child_branch.child_branches[0].as_tuple()
248 self.assertEqual("zoo", location)
249 self.check_recipe_branch(child_branch, "zam", "lp:zam")
250
251@@ -286,11 +286,13 @@
252 + "merge zam lp:zam 2")
253 self.check_base_recipe_branch(base_branch, "http://foo.org/",
254 num_child_branches=2, revspec="revid:a")
255- child_branch, location = base_branch.child_branches[0]
256+ instruction = base_branch.child_branches[0]
257+ child_branch = instruction.recipe_branch
258+ location = instruction.nest_path
259 self.assertEqual("baz", location)
260 self.check_recipe_branch(child_branch, "bar", "http://bar.org",
261 revspec="tag:b")
262- child_branch, location = base_branch.child_branches[1]
263+ child_branch, location = base_branch.child_branches[1].as_tuple()
264 self.assertEqual(None, location)
265 self.check_recipe_branch(child_branch, "zam", "lp:zam", revspec="2")
266
267@@ -300,7 +302,7 @@
268 + "run touch test \n")
269 self.check_base_recipe_branch(base_branch, "http://foo.org/",
270 num_child_branches=1)
271- child_branch, command = base_branch.child_branches[0]
272+ child_branch, command = base_branch.child_branches[0].as_tuple()
273 self.assertEqual(None, child_branch)
274 self.assertEqual("touch test", command)
275

Subscribers

People subscribed via source and target branches