On 07/12/2016 06:59 AM, Alexander Berntsen wrote:
> The _chf_deserializers and _md5_deserializer stuff looks rather
> overengineered. 

That stuff is not strictly required after the addition of the
intelligent reconstruct_eclasses skipping in __getitem__. However, it's
still good to have because it protects against a subtle misbehavior of
reconstruct_eclasses, where it's called with chf_type='md5' and produces
an invalid data structure containing mtime strings (rather than mtime ints).

> I don't know what the reconstruct_eclasses skipping
> entails (the comment makes it seem like "skip this because apparently
> it's different lol who knows -_o_-").

The _eclasses_data contains either md5 or mtime. It's a waste of time to
try to call reconstruct_eclasses with chf_type='md5' when eclasses
contains mtime data (and it would also produce an invalid data structure
in the absence of the _chf_deserializers and _md5_deserializer stuff).
So, it's nice to take the presence of _md5_ or _mtime_ in the cache
entry as a hint about whether _eclasses_ contains md5 or mtime data.

> The rest of the patch lgtm.

I'll add the insights that I have discussed above as comments the patch.

Reply via email to