Code review comment for lp:~al-maisan/launchpad/changes-url-474876

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good, with the addition of the comment and unit test as we
discussed. Thanks!

<allenap> al-maisan_: Line 332, does getPublishedSources() sort the results?
<al-maisan_> allenap: yes, it does.
<allenap> al-maisan_: getChangesFilesForSources() seems to now only be used in lib/lp/soyuz/adapters/archivesourcepublication.py. Is there any way to change that to use your new query?
<al-maisan_> allenap: I am not sure, that method is selecting and pre-joining a lot more data and that probably makes sense for the web UI
<allenap> al-maisan_: Okay. Do you think you could comment in getChangesFilesForSources() that getChangesFileLFA() contains a potentially more efficient query, depending on what you need out of it.
<al-maisan_> allenap: sure, will do that.
<allenap> al-maisan_: There aren't any unit tests for getChangesFileLFA(). I can see that changes_file_url is tested in a few places, but it's not quite the same thing. Unless there's a good reason not to, could you add this?
<al-maisan_> allenap: good point, I'll add unit tests for getChangesFileLFA().

review: Approve

« Back to merge proposal