launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/code/model/githosting.py'
> --- lib/lp/code/model/githosting.py	2016-04-19 17:18:39 +0000
> +++ lib/lp/code/model/githosting.py	2016-04-29 22:08:19 +0000
> @@ -114,10 +115,9 @@
>                  logger.info(
>                      "Requesting merge diff for %s from %s to %s" % (
>                          path, base, head))
> -            url = "/repo/%s/compare-merge/%s:%s" % (path, base, head)
> -            if prerequisite is not None:
> -                url += "?sha1_prerequisite=%s" % prerequisite
> -            return self._get(url)
> +            url = "/repo/%s/compare-merge/%s:%s" % (
> +                path, quote(base), quote(head))
> +            return self._get(url, params={"sha1_prerequisite": prerequisite})

params is a nice abstraction to use here.  I see from looking at the code for requests that a value of None in the params dictionary results in the parameter being omitted.  I guess this behaviour is convenient in terms of brevity, but it's also AFAICS entirely undocumented in http://docs.python-requests.org/en/latest/api/ so seems like an implementation detail that could change under us in future.  I'd prefer this to be more explicit, something like this:

  params = {}
  if prerequisite is not None:
      params["sha1_prerequisite"] = prerequisite
  return self._get(url, params=params)

>          except Exception as e:
>              raise GitRepositoryScanFault(
>                  "Failed to get merge diff from Git repository: %s" %
> @@ -147,3 +147,15 @@
>          except Exception as e:
>              raise GitRepositoryDeletionFault(
>                  "Failed to delete Git repository: %s" % unicode(e))
> +
> +    def getBlob(self, path, filename, rev=None, logger=None):
> +        """See `IGitHostingClient`."""
> +        try:
> +            if logger is not None:
> +                logger.info(
> +                    "Fetching file %s from repository %s" % (filename, path))
> +            url = "/repo/%s/blob/%s" % (path, quote(filename))
> +            return self._get(url, params={"rev": rev})

Same here.

> +        except Exception as e:
> +            raise GitRepositoryScanFault(
> +                "Failed to get file from Git repository: %s" % unicode(e))


-- 
https://code.launchpad.net/~maxiberta/launchpad/git-get-blob/+merge/293337
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.