enthought / enthought/apptools

Usage of updater in VersionedUnpickler is undocumented and maybe redundant

Open
#238 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
38
Forks
21
PR merge metrics
No merged PRs in 30d

Description

The `VersionedUnpickler` accepts an updater object:
https://github.com/enthought/apptools/blob/632a4bfd7ce861b985a3d9dd8c25e44e5b433f0f/apptools/persistence/versioned_unpickler.py#L111-L113

This was supposed to be its "interface":
https://github.com/enthought/apptools/blob/632a4bfd7ce861b985a3d9dd8c25e44e5b433f0f/apptools/persistence/updater.py#L9-L17

But from where it is used, there are more features expected of this interface, e.g. it is expected to have an attribute called `setstates` which is a dictionary:
https://github.com/enthought/apptools/blob/632a4bfd7ce861b985a3d9dd8c25e44e5b433f0f/apptools/persistence/versioned_unpickler.py#L157

The structure of this dictionary is not documented (and there seem to be no tests for it). It seems to be used for monkeypatching a class `__setstate__` (a global state that does not belong to apptools!), but it may not be restored.
Monkeypatching here in the code path if `VersionedUnpickler.updater` is not None:
https://github.com/enthought/apptools/blob/632a4bfd7ce861b985a3d9dd8c25e44e5b433f0f/apptools/persistence/versioned_unpickler.py#L166-L167

To be restored if some other unpickler without an updater came across the same class:
https://github.com/enthought/apptools/blob/632a4bfd7ce861b985a3d9dd8c25e44e5b433f0f/apptools/persistence/versioned_unpickler.py#L145-L148

It is possible that no one uses an updater with the `VersionedUnpickler` so this monkeypatching is never exercised. I have not checked if there are other uses of an updater anywhere else. It could be a feature that can be removed.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.