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.
>
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: bazaar. launchpad. net/ or the lp: URL.
>
> + Either from the external specified in Branch.url, from the URL on
> + http://
>
> 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