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

Revision history for this message
James Westby (james-w) wrote :

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?

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

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?

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

Thanks,

James

review: Approve

« Back to merge proposal