launchpad-reviewers team mailing list archive

[Maximiliano Bertacchini <maximiliano.bertacchini@xxxxxxxxxxxxx>]

Just a quick reply to the previous comment.

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})

This behaviour is indeed not documented in the request's API docs, and I agree that explicit is better than implicit. It is mentioned in the Quickstart (http://docs.python-requests.org/en/master/user/quickstart/#passing-parameters-in-urls); and there's also a unit test that checks URL correctness when revision is None ('lp.code.model.tests.test_githosting:test_getBlob').

>          except Exception as e:
>              raise GitRepositoryScanFault(
>                  "Failed to get merge diff from Git repository: %s" %


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