launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+492719@xxxxxxxxxxxxxxxxxx>]

Thanks for the MP, this looks mostly good. I left some comments about some Python specifics and about how we could possibly even remove some more code.

Would you please update the commit message? The term refactoring is usually only used for implementation changes which do not affect the behavior. And your MP clearly changes the behavior by removing features.



Diff comments:

> diff --git a/lib/lp/code/browser/codeimport.py b/lib/lp/code/browser/codeimport.py
> index 9ea3dcf..60175d1 100644
> --- a/lib/lp/code/browser/codeimport.py
> +++ b/lib/lp/code/browser/codeimport.py
> @@ -409,36 +365,19 @@ class CodeImportNewView(CodeImportBaseView, CodeImportNameValidationMixin):
>          # display them separately in the form.
>          soup = BeautifulSoup(self.widgets["rcs_type"]())
>          fields = soup.find_all("input")
> -        [cvs_button, svn_button, git_button, bzr_button, empty_marker] = [
> -            field
> -            for field in fields
> -            if field.get("value") in ["CVS", "BZR_SVN", "GIT", "BZR", "1"]
> +        [git_button, empty_marker] = [
> +            field for field in fields if field.get("value") in ["GIT", "1"]
>          ]
> -        bzr_button["onclick"] = "updateWidgets()"
> -        cvs_button["onclick"] = "updateWidgets()"
> -        svn_button["onclick"] = "updateWidgets()"
>          git_button["onclick"] = "updateWidgets()"
>          # The following attributes are used only in the page template.
> -        self.rcs_type_cvs = str(cvs_button)
> -        self.rcs_type_svn = str(svn_button)
>          self.rcs_type_git = str(git_button)
> -        self.rcs_type_bzr = str(bzr_button)
>          self.rcs_type_emptymarker = str(empty_marker)
> -        # This widget is only conditionally required in the rcs_type == GIT
> -        # case, but we still don't want a "(nothing selected)" item.
> -        self.widgets["git_target_rcs_type"]._displayItemForMissingValue = False
>  
>      def _getImportLocation(self, data):
>          """Return the import location based on type."""
>          rcs_type = data["rcs_type"]
> -        if rcs_type == RevisionControlSystems.CVS:
> -            return data.get("cvs_root"), data.get("cvs_module"), None
> -        elif rcs_type == RevisionControlSystems.BZR_SVN:
> -            return None, None, data.get("svn_branch_url")
> -        elif rcs_type == RevisionControlSystems.GIT:
> -            return None, None, data.get("git_repo_url")
> -        elif rcs_type == RevisionControlSystems.BZR:
> -            return None, None, data.get("bzr_branch_url")
> +        if rcs_type == RevisionControlSystems.GIT:
> +            return data.get("git_repo_url")

It is a bit hard to check this on the train on my 13inch laptop.. but is this check still necessary? Is it even possible that wrong values get passed in here?

>          else:
>              raise AssertionError(
>                  "Unexpected revision control type %r." % rcs_type
> @@ -447,23 +386,19 @@ class CodeImportNewView(CodeImportBaseView, CodeImportNameValidationMixin):
>      def _create_import(self, data, status):
>          """Create the code import."""
>          product = self.getProduct(data)
> -        cvs_root, cvs_module, url = self._getImportLocation(data)
> -        if data["rcs_type"] == RevisionControlSystems.GIT:
> -            target_rcs_type = data.get(
> -                "git_target_rcs_type", TargetRevisionControlSystems.BZR
> -            )
> -        else:
> -            target_rcs_type = TargetRevisionControlSystems.BZR
> +        url = self._getImportLocation(data)
> +        assert data["rcs_type"] == RevisionControlSystems.GIT

In Python you should not use `assert` in production code, as this could be stripped away when running Python in optimized mode.

Usually, `assert` is only used in tests when using pytest.

Also, it seems we do this same check kind of twice, here and just in the changeset above.

IMHO - check the incoming data at the outermost layer, so these checks do not drag into our system.

> +
>          return getUtility(ICodeImportSet).new(
>              registrant=self.user,
>              owner=data["owner"],
>              context=product,
>              branch_name=data["branch_name"],
>              rcs_type=data["rcs_type"],
> -            target_rcs_type=target_rcs_type,
> +            target_rcs_type=TargetRevisionControlSystems.GIT,

If we have only one kind of target revision control system, do we still need this argument? Or could we remove it here - and in the ICodeImportSet?

>              url=url,
> -            cvs_root=cvs_root,
> -            cvs_module=cvs_module,
> +            cvs_root=None,
> +            cvs_module=None,

Do we need to keep those?

>              review_status=status,
>          )
>  
> @@ -504,20 +439,12 @@ class CodeImportNewView(CodeImportBaseView, CodeImportNameValidationMixin):
>  
>      def validate_widgets(self, data, names=None):
>          """See `LaunchpadFormView`."""
> -        self.widgets["git_target_rcs_type"].context.required = (
> -            data.get("rcs_type") == RevisionControlSystems.GIT
> -        )
>          super().validate_widgets(data, names=names)
>  
>      def validate(self, data):
>          """See `LaunchpadFormView`."""
>          rcs_type = data["rcs_type"]
> -        if rcs_type == RevisionControlSystems.GIT:
> -            target_rcs_type = data.get(
> -                "git_target_rcs_type", TargetRevisionControlSystems.BZR
> -            )
> -        else:
> -            target_rcs_type = TargetRevisionControlSystems.BZR
> +        assert rcs_type == RevisionControlSystems.GIT

see above

>  
>          # Make sure that the user is able to create branches/repositories
>          # for the specified namespace.
> @@ -542,29 +464,13 @@ class CodeImportNewView(CodeImportBaseView, CodeImportNameValidationMixin):
>  
>          # Make sure fields for unselected revision control systems
>          # are blanked out:
> -        if rcs_type == RevisionControlSystems.CVS:
> -            self._validateCVS(data.get("cvs_root"), data.get("cvs_module"))
> -        elif rcs_type == RevisionControlSystems.BZR_SVN:
> -            self._validateURL(
> -                data.get("svn_branch_url"),
> -                rcs_type,
> -                target_rcs_type,
> -                field_name="svn_branch_url",
> -            )
> -        elif rcs_type == RevisionControlSystems.GIT:
> +        if rcs_type == RevisionControlSystems.GIT:
>              self._validateURL(
>                  data.get("git_repo_url"),
>                  rcs_type,
> -                target_rcs_type,

see above

> +                target_rcs_type=TargetRevisionControlSystems.GIT,
>                  field_name="git_repo_url",
>              )
> -        elif rcs_type == RevisionControlSystems.BZR:
> -            self._validateURL(
> -                data.get("bzr_branch_url"),
> -                rcs_type,
> -                target_rcs_type,
> -                field_name="bzr_branch_url",
> -            )
>          else:
>              raise AssertionError(
>                  "Unexpected revision control type %r." % rcs_type
> diff --git a/lib/lp/code/model/codeimport.py b/lib/lp/code/model/codeimport.py
> index 5ba1ed7..9dea434 100644
> --- a/lib/lp/code/model/codeimport.py
> +++ b/lib/lp/code/model/codeimport.py
> @@ -338,61 +335,35 @@ class CodeImportSet:
>          owner=None,
>      ):
>          """See `ICodeImportSet`."""
> -        if rcs_type == RevisionControlSystems.CVS:
> -            assert cvs_root is not None and cvs_module is not None
> -            assert url is None
> -        elif rcs_type in NON_CVS_RCS_TYPES:
> -            assert cvs_root is None and cvs_module is None
> -            assert url is not None
> -        else:
> -            raise AssertionError(
> -                "Don't know how to sanity check source details for unknown "
> -                "rcs_type %s" % rcs_type
> -            )
> +        if (
> +            rcs_type != RevisionControlSystems.GIT
> +            and target_rcs_type != TargetRevisionControlSystems.GIT
> +        ):
> +            raise AssertionError("Unsupported rcs system")
> +        assert cvs_root is None and cvs_module is None
> +        assert url is not None

see above

>          if owner is None:
>              owner = registrant
> -        if target_rcs_type == TargetRevisionControlSystems.BZR:
> -            # XXX cjwatson 2016-10-15: Testing
> -            # IHasBranches.providedBy(context) would seem more in line with
> -            # the Git case, but for some reason ProductSeries doesn't
> -            # provide that.  We should sync this up somehow.
> -            try:
> -                target = IBranchTarget(context)
> -            except TypeError:
> -                raise CodeImportInvalidTargetType(context, target_rcs_type)
> -            namespace = target.getNamespace(owner)
> -        elif target_rcs_type == TargetRevisionControlSystems.GIT:
> -            if not IHasGitRepositories.providedBy(context):
> -                raise CodeImportInvalidTargetType(context, target_rcs_type)
> -            if rcs_type != RevisionControlSystems.GIT:
> -                raise AssertionError(
> -                    "Can't import rcs_type %s into a Git repository" % rcs_type
> -                )
> -            target = namespace = get_git_namespace(context, owner)
> -        else:
> +        if not IHasGitRepositories.providedBy(context):
> +            raise CodeImportInvalidTargetType(context, target_rcs_type)
> +        if rcs_type != RevisionControlSystems.GIT:
>              raise AssertionError(
> -                "Can't import to target_rcs_type %s" % target_rcs_type
> +                "Can't import rcs_type %s into a Git repository" % rcs_type
>              )
> +        target = namespace = get_git_namespace(context, owner)
>          if review_status is None:
>              # Auto approve imports.
>              review_status = CodeImportReviewStatus.REVIEWED
>          if not target.supports_code_imports:
>              raise AssertionError("%r doesn't support code imports" % target)
>          # Create the branch for the CodeImport.
> -        if target_rcs_type == TargetRevisionControlSystems.BZR:
> -            import_target = namespace.createBranch(
> -                branch_type=BranchType.IMPORTED,
> -                name=branch_name,
> -                registrant=registrant,
> -            )
> -        else:
> -            import_target = namespace.createRepository(
> -                repository_type=GitRepositoryType.IMPORTED,
> -                name=branch_name,
> -                registrant=registrant,
> -            )
> -            hosting_path = import_target.getInternalPath()
> -            getUtility(IGitHostingClient).create(hosting_path)
> +        import_target = namespace.createRepository(
> +            repository_type=GitRepositoryType.IMPORTED,
> +            name=branch_name,
> +            registrant=registrant,
> +        )
> +        hosting_path = import_target.getInternalPath()
> +        getUtility(IGitHostingClient).create(hosting_path)
>  
>          code_import = CodeImport(
>              registrant=registrant,
> diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py
> index 13a750c..8d9b37d 100644
> --- a/lib/lp/registry/scripts/tests/test_closeaccount.py
> +++ b/lib/lp/registry/scripts/tests/test_closeaccount.py
> @@ -826,10 +826,7 @@ class TestCloseAccount(TestCaseWithFactory):
>              self.factory.makeCodeImport(
>                  registrant=person, target_rcs_type=target_rcs_type, owner=team
>              )
> -            for target_rcs_type in (
> -                TargetRevisionControlSystems.BZR,
> -                TargetRevisionControlSystems.GIT,
> -            )
> +            for target_rcs_type in (TargetRevisionControlSystems.GIT,)

I don't think we need a for loop for a single item, but the MP diff view does not show enough context.

>          ]
>          person_id = person.id
>          account_id = person.account.id


-- 
https://code.launchpad.net/~finnrg/launchpad/+git/launchpad/+merge/492719
Your team Launchpad code reviewers is requested to review the proposed merge of ~finnrg/launchpad:refactor/remove-bzr-codeimports into launchpad:master.