Code review comment for lp:~kissiel/checkbox/suspend-v7

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

16:56 <@zyga> kissiel: I'd like to also see patches that make use fo the new field where it makes sense to do so (SA APIs)
16:57 < kissiel> zyga: I used datetime at first go, but this required custom endcoders and decoders
16:57 < kissiel> this is why I changed that to posix
16:58 <@zyga> kissiel: that's okay, we can handle that on POD assignment
16:59 <@zyga> kissiel: timestamp is infinitely more useful
16:59 <@zyga> kissiel: when it carries a human readable time concept
16:59 <@zyga> kissiel: not when it carries a number
16:59 < kissiel> zyga: so were should the parsing happen?
16:59 < kissiel> zyga: in your opinion?
16:59 <@zyga> kissiel: you can fix that in one place, in the resume code that deserializes session meta-data
16:59 <@zyga> kissiel: it does assignment anyway
17:00 < kissiel> zyga: there's more than 1 place where json.loads happne
17:00 < kissiel> *happen
17:00 <@zyga> kissiel: can you be more specific?
17:00 <@zyga> this field is _assigned_ in one place
17:00 <@zyga> kissiel: json decode can and should just decode a string
17:01 <@zyga> kissiel: there is (and should be) only one place that assigns session_state.metadata.timestsamp
17:01 <@zyga> *stamp
17:01 < kissiel> mkay
17:01 < kissiel> I think I know what you mean
17:01 <@zyga> kissiel: fantastic, thanks
17:01 < kissiel> this will make use of the default json decoder
17:01 <@zyga> kissiel: well, just decode as before, but re-interpret that one field

« Back to merge proposal