Code review comment for lp:~rodsmith/checkbox/network-fixes

Revision history for this message
Daniel Manrique (roadmr) wrote :

Thanks! Time permitting, do have a look at Python's iterator handling, you seldom need to manage incrementing a counter yourself. If you ever have some spare time and want to refactor this to use what I suggested, it would be a fun exercise.

http://www.diveintopython.net/file_handling/for_loops.html

Also, bear in mind that opening any file can throw an exception. The assumption that /dev/null can be successfully opened is reasonable, as is the one about TemporaryFile() being mostly likely to succeed; if they do fail, a very clear trace will be thrown and that will help debug the problem. But usually it's good practice to try to catch those file opening exceptions. I won't block this merge on that though, so we're golden in that respect.

With that in mind, there's only one tiny change I'd ask you to make (it slipped by both of us!). See below. Once that's in, we can proceed to approve and merge this.

review: Needs Fixing

« Back to merge proposal