launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

In addition to the comments below, please could you add some tests for this?

Diff comments:

> 
> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py	2016-04-14 05:16:26 +0000
> +++ lib/lp/registry/browser/product.py	2016-07-12 06:34:35 +0000
> @@ -1743,6 +1745,14 @@
>                  self.context.pillar))
>  
>      @property
> +    def git_url(self):

This should be git_ssh_url or similar.

> +        base_url = urlparse.urlparse(
> +            urlutils.join(config.codehosting.git_ssh_root, self.context.name))
> +        url = base_url._replace(
> +            netloc="{}@{}".format(self.user.name, base_url.hostname))
> +        return url.geturl()

_replace is only documented as being a method of namedtuple, and urlparse/urlsplit are not documented as returning specifically a namedtuple, so we shouldn't rely on this.  I'd suggest instead:

  from urlparse import urlunsplit

  from lp.services.webapp.url import urlsplit

  base_url = urlsplit(
      urlutils.join(config.codehosting.git_ssh_root, self.context.name))
  url = list(base_url)
  url[1] = "{}@{}".format(self.user.name, base_url.hostname)
  return urlunsplit(url)

> +
> +    @property
>      def next_url(self):
>          """Return the next_url.
>  


-- 
https://code.launchpad.net/~blr/launchpad/bug-1600055-configure-code-remote-username/+merge/299774
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.