Code review comment for lp:~jml/udd/symbol-import

On Wed, Oct 5, 2011 at 4:57 PM, James Westby <email address hidden> wrote:
> Review: Approve
>
> Hi,
>
> Thanks for the branch, there are some nice cleanups in here to sweeten
> the deal.
>
> 56      + if include_debian:
> 57      + for s in debian.series:
> 58      + collection = lpapi.lp_call(lpapi.call_with_limited_size,
> 59      + d_archive.getPublishedSources,
> 60      + status="Pending",
> 61      + distro_series=s, created_since_date=last_update)
> 62      + for publication in lpapi.iterate_collection(collection):
> 63      + packages = {}
> 64      + if publication.source_package_name in packages:
> 65      + continue
> 66      + packages[publication.source_package_name] = 0
> 67      + db.update_packages(packages)
>
> Would this be better as a list of distribution names, rather than a boolean?
>

Yeah, that's how I started off, and then I saw just how much 'debian'
and 'ubuntu' were spread throughout the code. (e.g., the manually
maintained distro → series maps).

> Likely someone is going to want to use this code with derived distros
> at some point, and that change would seem to be more inline with that.
>
> I'm not saying that you have to support derived distros in every way though :-)
>

OK. I'll do what I can and see what turns up.

>
> I'm interested in why you changed import in one file from "from datetime import datetime"
> to "import datetime" and changed another file in the opposite direction?
>

Umm, I didn't?

>
> I'm sure this comment is going to annoy you, but...
>
> 367     - request = operation.wadl_method.request
>
> I think that line should be there, as that code is taken from lazr.restfulclient,
> and someone in the future looking to see if we can drop the code will have
> to wonder if removing that dead code was significant.
>
> It even annoys me :-)
>

OK. I didn't see a comment there saying it was taken from
lazr.restfulclient. I'll add a comment along those lines when I
restore the removed line.

Thanks!
jml