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

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

On Wed, 05 Oct 2011 16:07:25 -0000, Jonathan Lange <email address hidden> wrote:
> 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).

Ah, that's true. They are a pain, but I don't know of a way to get rid
of them.

Perhaps they aren't needed for anything no in ('debian', 'ubuntu')
though.

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

We can always deprecate the debian option later, so if it's too much
work we'll just do that.

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

Sorry, I have no idea where I saw that, my mistake entirely.

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

Good idea, thanks.

James

« Back to merge proposal