Code review comment for ~nacc/git-ubuntu:importer-cleanup-tmpdir-always

Revision history for this message
Robie Basak (racb) wrote :

Sorry I've been so long in responding to this. I was tasked on other projects. I've finally been able to context switch back to git-ubuntu and hope to be on it for the next week or so.

I've made a bunch of comments inline. I'm sorry I didn't notice the kwargs thing before; I realise that was present in my original review.

In any case, given that it's been so long, I don't think it's fair to ask you to take all of this on again. So I've prepared a branch that addresses my comments, which I can post as a separate MP and supersede this MP if you're happy with that as an approach to proceeding?

In that branch I've also written a proposed replacement for your test_importer_main_cleanup_on_exception called test_importer_close_repository_on_exception. I've left test_importer_main_cleanup_on_exception there for now, but I propose (weakly) that it can be dropped because test_importer_close_repository_on_exception is simpler and combined with your unit tests on GitUbuntuRepository.close() establish the same regression check that test_importer_main_cleanup_on_exception does.

I've pushed my branch to https://code.launchpad.net/~racb/usd-importer/+git/usd-importer/+ref/importer-cleanup-tmpdir-always - feel free to resubmit this MP using that branch if you'd like, or I can do it, or we can find some other way forward.

review: Needs Fixing

« Back to merge proposal