launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-branchtarget-newcodeimport into lp:launchpad with lp:~cjwatson/launchpad/refactor-code-import-context as a prerequisite.

Commit message:
Get rid of IBranchTarget.newCodeImport and use ICodeImportSet.new directly.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-branchtarget-newcodeimport/+merge/307419

Get rid of IBranchTarget.newCodeImport and use ICodeImportSet.new directly.  The intermediate layer wasn't pulling its weight, and it would have unnecessarily complicated the process of adding git-targeting imports.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-branchtarget-newcodeimport into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branchtarget.py'
--- lib/lp/code/interfaces/branchtarget.py	2016-01-12 13:43:34 +0000
+++ lib/lp/code/interfaces/branchtarget.py	2016-10-03 11:00:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for branch targets.
@@ -122,23 +122,6 @@
     def getBugTask(bug):
         """Get the BugTask for a given bug related to the branch target."""
 
-    def newCodeImport(registrant, branch_name, rcs_type, url=None,
-                      cvs_root=None, cvs_module=None, owner=None):
-        """Create a new code import for this target.
-
-        :param registrant: the `IPerson` who should be recorded as creating
-            the import and will own the resulting branch.
-        :param branch_name: the name the resulting branch should have.
-        :param rcs_type: the type of the foreign VCS.
-        :param url: the url to import from if the import isn't CVS.
-        :param cvs_root: if the import is from CVS the CVSROOT to import from.
-        :param cvs_module: if the import is from CVS the module to import.
-        :param owner: the `IPerson` to own the resulting branch, or None to
-            use registrant.
-        :returns: an `ICodeImport`.
-        :raises AssertionError: if supports_code_imports is False.
-        """
-
     def getRelatedSeriesBranchInfo(parent_branch, limit_results=None):
         """Find development branch info related to this parent branch.
 

=== modified file 'lib/lp/code/model/branchtarget.py'
--- lib/lp/code/model/branchtarget.py	2016-10-03 11:00:41 +0000
+++ lib/lp/code/model/branchtarget.py	2016-10-03 11:00:44 +0000
@@ -22,7 +22,6 @@
     check_default_stacked_on,
     IBranchTarget,
     )
-from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.linkedbranch import get_linked_to_branch
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -39,13 +38,6 @@
     def __ne__(self, other):
         return self.context != other.context
 
-    def newCodeImport(self, registrant, branch_name, rcs_type, url=None,
-                      cvs_root=None, cvs_module=None, owner=None):
-        """See `IBranchTarget`."""
-        return getUtility(ICodeImportSet).new(
-            registrant, self.context, branch_name, rcs_type, url=url,
-            cvs_root=cvs_root, cvs_module=cvs_module, owner=owner)
-
     def getRelatedSeriesBranchInfo(self, parent_branch, limit_results=None):
         """See `IBranchTarget`."""
         return []

=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py	2015-09-30 09:05:08 +0000
+++ lib/lp/code/model/hasbranches.py	2016-10-03 11:00:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Mixin classes to implement methods for IHas<code related bits>."""
@@ -22,7 +22,7 @@
     IAllBranches,
     IBranchCollection,
     )
-from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.gitcollection import (
     IAllGitRepositories,
     IGitCollection,
@@ -129,6 +129,6 @@
             rcs_type=None, url=None, cvs_root=None, cvs_module=None,
             owner=None):
         """See `IHasCodeImports`."""
-        return IBranchTarget(self).newCodeImport(registrant, branch_name,
-                rcs_type, url=url, cvs_root=cvs_root, cvs_module=cvs_module,
-                owner=owner)
+        return getUtility(ICodeImportSet).new(
+            registrant, self, branch_name, rcs_type, url=url,
+            cvs_root=cvs_root, cvs_module=cvs_module, owner=owner)

=== modified file 'lib/lp/code/model/tests/test_branchtarget.py'
--- lib/lp/code/model/tests/test_branchtarget.py	2016-01-12 13:43:34 +0000
+++ lib/lp/code/model/tests/test_branchtarget.py	2016-10-03 11:00:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for branch contexts."""
@@ -8,12 +8,8 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
-from lp.code.enums import (
-    BranchType,
-    RevisionControlSystems,
-    )
+from lp.code.enums import BranchType
 from lp.code.interfaces.branchtarget import IBranchTarget
-from lp.code.interfaces.codeimport import ICodeImport
 from lp.code.model.branchtarget import (
     check_default_stacked_on,
     PackageBranchTarget,
@@ -201,20 +197,6 @@
     def test_supports_code_imports(self):
         self.assertTrue(self.target.supports_code_imports)
 
-    def test_creating_code_import_succeeds(self):
-        target_url = self.factory.getUniqueURL()
-        branch_name = self.factory.getUniqueString("name-")
-        owner = self.factory.makePerson()
-        code_import = self.target.newCodeImport(
-            owner, branch_name, RevisionControlSystems.GIT, url=target_url)
-        code_import = removeSecurityProxy(code_import)
-        self.assertProvides(code_import, ICodeImport)
-        self.assertEqual(target_url, code_import.url)
-        self.assertEqual(branch_name, code_import.branch.name)
-        self.assertEqual(owner, code_import.registrant)
-        self.assertEqual(owner, code_import.branch.owner)
-        self.assertEqual(self.target, code_import.branch.target)
-
     def test_allow_recipe_name_from_target(self):
         self.assertTrue(self.target.allow_recipe_name_from_target)
 
@@ -331,13 +313,6 @@
     def test_doesnt_support_code_imports(self):
         self.assertFalse(self.target.supports_code_imports)
 
-    def test_creating_code_import_fails(self):
-        self.assertRaises(
-            AssertionError, self.target.newCodeImport,
-                self.factory.makePerson(),
-                self.factory.getUniqueString("name-"),
-                RevisionControlSystems.GIT, url=self.factory.getUniqueURL())
-
     def test_does_not_allow_recipe_name_from_target(self):
         self.assertFalse(self.target.allow_recipe_name_from_target)
 
@@ -462,20 +437,6 @@
     def test_supports_code_imports(self):
         self.assertTrue(self.target.supports_code_imports)
 
-    def test_creating_code_import_succeeds(self):
-        target_url = self.factory.getUniqueURL()
-        branch_name = self.factory.getUniqueString("name-")
-        owner = self.factory.makePerson()
-        code_import = self.target.newCodeImport(
-            owner, branch_name, RevisionControlSystems.GIT, url=target_url)
-        code_import = removeSecurityProxy(code_import)
-        self.assertProvides(code_import, ICodeImport)
-        self.assertEqual(target_url, code_import.url)
-        self.assertEqual(branch_name, code_import.branch.name)
-        self.assertEqual(owner, code_import.registrant)
-        self.assertEqual(owner, code_import.branch.owner)
-        self.assertEqual(self.target, code_import.branch.target)
-
     def test_allow_recipe_name_from_target(self):
         self.assertTrue(self.target.allow_recipe_name_from_target)