Code review comment for lp:~jml/launchpad/searchTasks-modified-since-590535

Revision history for this message
Deryck Hodge (deryck) wrote :

Hi, Jono.

This branch looks good to me. I was going to suggest changing the test name to be less generic about search and more about the specific case you were covering, but realizing we have no unit test coverage for searchTasks, I'm fine with this now, even if there is some tiny duplication. I would like to see us eventually move tests out of doc/bugtask-search.txt and into this class, where appropriate. Given that, keeping the name seems appropriate.

Also, limiting to tasks after but not including modified_since feels right to me, given the naming. The way you've done it here is what I would expect when using the parameter.

Finally, I agree with your choice to simply demonstrate the webservice and rely on unit test coverage. I have taken this approach myself recently.

Thanks for the work!

Cheers,
deryck

review: Approve (code)

« Back to merge proposal