Author davin
Recipients advance512, davin, josh.r, pitrou, sbt, serhiy.storchaka
Date 2015-03-13.03:05:54
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1426215956.06.0.20448193164.issue23051@psf.upfronthosting.co.za>
In-reply-to
Content
After pondering it for two days and coming back to it with hopefully "fresh eyes", I believe that changing the for-loop to a while-loop is not overall easier to understand -- I would lean towards keeping the for-loop.

I do think the change to the while-loop very much made the exception handling logic clearer but seeing the while-loop with the "manual" invocation of next and incrementing of the variable i and re-use of i as a signal to break out of a loop (setting i = None) made other things less clear.  My belief is that someone reading this method's code for the first time will read the for-loop version as, "try to loop through the enumerated tasks and if anything goes wrong then set the next position in the cache to 'failed'".  That top-level reading is, I think, not quite as easy with the while-loop.  Without the exception handling that we add in this patch, the original code used the for-loop and would, I think, have looked weird if it had tried to use a while-loop -- I think that's a sign that the for-loop is likely to be more easily understood by a first-time reader.

Though I am not sure it really matters, the while-loop version would only help end the processing of further jobs if an exception occurs in the iterator whereas the for-loop version might help if exceptions occur in a couple of other places.  We do not have a clear motivation for needing that however.
History
Date User Action Args
2015-03-13 03:05:56davinsetrecipients: + davin, pitrou, sbt, serhiy.storchaka, josh.r, advance512
2015-03-13 03:05:56davinsetmessageid: <1426215956.06.0.20448193164.issue23051@psf.upfronthosting.co.za>
2015-03-13 03:05:56davinlinkissue23051 messages
2015-03-13 03:05:54davincreate