Code review comment for lp:~spiv/bzr-usertest/network-suite

Revision history for this message
Martin Pool (mbp) wrote :

 class AnotherBranchTask(ScriptTask):

+ def __init__(self, branch_name='-anotherBranch'):
+ self._branch_name = branch_name
+ super(AnotherBranchTask, self).__init__()
+
+ def id(self):
+ return super(AnotherBranchTask, self).id() + self._branch_name
+
+ def name(self):
+ return super(AnotherBranchTask, self).name() + self._branch_name
+
     def load(self):
         self.compile_default("""
             cd ..
- $tool clone $repo_read_url/trunk ${work_basename}-anotherBranch
- cd ${work_basename}-anotherBranch
- """)
- # Go up an extra level for Bazaar to get outside the shared repo
- self.compile_for_tools([TOOL_BZR], """
- cd ..
- cd ..
- $tool clone $repo_read_url/trunk ${work_basename}-anotherBranch
- cd ${work_basename}-anotherBranch
- """)
+ $tool clone $repo_read_url/trunk ${work_basename}%(branch_name)s
+ cd ${work_basename}%(branch_name)s
+ """ % {'branch_name': self._branch_name})

So it looks like you're doing variable substitution into this string through both ${} and %()s. That's not necessarily wrong and it seems syntactically valid, but I do wonder if it's good style. Is there a reason you can't feed the branch name in through the same mechanism that the rest of usertest apparently uses?

review: Needs Fixing (none)

« Back to merge proposal