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

On Thu, Nov 19, 2009 at 11:19 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> Review: Approve code
> Looks fine overall, and with a few nice drive-by improvements.  A few smaller things:
>

Thanks.

> 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 ".
>

Changed.

> 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.
>

Good idea. Done.

> 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.
>

Removed.

jml