Launchpad itself

Code review comment for lp:~jml/launchpad/get-by-urls

  1. get-by-urls
  2. Merge into devel
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks fine overall, and with a few nice drive-by improvements. A few smaller things:

Docstring, line 86 of the diff:

+ Either from the external specified in Branch.url, from the URL on
+ http://bazaar.launchpad.net/ or the lp: URL.

I know it's ugly after a URL, but consider replacing the
    " or "
with
    ", or from ".

Also, if there's any real cost associated with a lookup, you might want to do the lookup for set(urls) instead of urls in branchlookup.py (line 146 of the diff), just in case you get a lot of insane requests with duplicated URLs.

Finally, in line 130 of the diff:

+ # XXX: Move this to IBranchLookup

Does that still make sense? AFAICS you already have. If so, remove the comment; if not, file a bug and include its number in the comment.

Jeroen

review: Approve (code)

« Back to merge proposal