Code review comment for lp:~kissiel/checkbox/fix-1397109

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

10:24 < kissiel> zyga, https://code.launchpad.net/~kissiel/checkbox/fix-1397109/+merge/243503 :)
10:25 <@zyga> kissiel: hmm, interesting
10:26 <@zyga> kissiel: I think it might be useful to keep those around indefinitely, I would certainly not remove them without first informing the
              user or let them choose
10:26 <@zyga> kissiel: the code itself looks okay
10:26 <@zyga> kissiel: load_checkpoint can raise exceptions
10:26 <@zyga> kissiel: IIRC
10:27 <@zyga> kissiel: I'd move the try/catch so that it covers only the .peek() call
10:27 <@zyga> kissiel: and move the garbage_flags set outside the loop
10:27 <@zyga> kissiel: intersection can be *probably* spelled as & for sets
10:27 <@zyga> kissiel: you may want to remove stuff you cannot peek either
10:28 < kissiel> zyga, I thought of that
10:28 < kissiel> zyga, but this would mean, that broken sessions are lost
10:28 < kissiel> zyga, and we/some1 might want to recover those
10:29 <@zyga> kissiel: yes, that's a good point, though some of those sessions are not broken (incompatible job error)
10:29 <@zyga> kissiel: just no longer resumable
10:29 <@zyga> ...
10:29 <@zyga> though
10:29 <@zyga> I'm wrong
10:29 <@zyga> peek doens't have that
10:29 <@zyga> only stuff like broken json/zip can kill peek
10:30 <@zyga> kissiel: oh, and missing i18n :D
10:30 <@zyga> kissiel: let's iterate and we'll se
10:30 <@zyga> see
10:30 < kissiel> zyga, cool, tahnks

« Back to merge proposal