7
\$\begingroup\$

Use case: I sometimes write programs that want to store a little state between runs, e.g., "What was the last file selected last time this program ran?" I wanted a convenient way to track this state. Python's built-in shelve module is pretty good but I wanted a way to restrict the possible set of keys in the shelf so that I wouldn't (due to a bag) store something under some unexpected key that would never be read. While I was at it I made it so the you access values using foo.a instead of foo['a'], though I paid a price for this.

Questions I have about this code are:

  1. Is there an easier way I could have accomplished this using built-in stuff or widely used libraries?
  2. Did I choose the right design?
    1. I could have tried to inherit from one of the existing shelf classes, though the shelve module decides at runtime what sort of shelf you should get back from shelve.open.
    2. I could have tried to use descriptors to create my own pseudo-properties, sort of like what's done here, but I don't see a need to decide which fields to include at the class, rather than instance, level
  3. Is there any less gross for me to use __getattr__ and __setattr__ (without having to store a list of fields to be handled as special)?
  4. Did I handle the nested context managers (shelf within RestrictedShelf) correctly? Do I need a bunch of try/except logic in __exit__, as in the fourth example in this answer?
  5. Should I be defining my own exception classes, identical with Exception beyond the class name, as I did?
  6. Is making _check_special_keys a class method, rather than a static method, the correct approach?

import shelve
import os
import sys

class UnexpectedKeyError(Exception):
    pass
class BadSpecialKey(Exception):
    pass

class RestrictedShelf(object):
    _SPECIAL_KEYS = ["_filepath", "_keys", "_shelf"]

    @classmethod
    def _check_special_keys(cls):
        for key in cls._SPECIAL_KEYS:
            if not key.startswith("_"):
                raise BadSpecialKey("Special key does not start with underscore: '{}'".format(key))

    def __init__(self, filepath, keys):
        self._filepath = filepath
        self._keys = list(keys) # Make a copy in case someone modifies the original list

        for key in keys:
            if not isinstance(key, str):
                raise TypeError("String object expected for key, {} found".format(key.__class__.__name__))
            # The keys in _SPECIAL_KEYS can't be used, but let's just rule out all keys that start with underscore for good measure
            if key.startswith("_"):
                raise ValueError("Key illegally starts with underscore: '{}'".format(key))

    def __enter__(self):
        self._shelf = shelve.open(self._filepath)
        for key in self._shelf.keys():
            if key not in self._keys:
                # This is not quite the same thing as a regular KeyError
                raise UnexpectedKeyError(key, "Shelf at '{}' contains unexpected key '{}'".format(self._filepath, key))
        return self

    def __exit__(self, exc_type, exc_val, exc_tb):
        if sys.version_info[0] <= 2:
            self._shelf.close()
        else:
            self._shelf.__exit__(exc_type, exc_val, exc_tb)

    def __getattr__(self, key):
        self.check_key(key)
        return self._shelf.get(key)

    def __setattr__(self, key, value):
        if key in self._SPECIAL_KEYS:
            # https://stackoverflow.com/a/7042247/2829764
            super(RestrictedShelf, self).__setattr__(key, value)
        else:
            self.check_key(key)
            self._shelf[key] = value

    def check_key(self, key):
        if key not in self._keys:
            raise KeyError(key)

RestrictedShelf._check_special_keys()


if __name__ == "__main__":
    with RestrictedShelf(os.path.expanduser("~/tmp/test.shelf"), "a b c".split()) as rs:
        print(rs.a) # `None` the first time you run this; `10` thereafter
        rs.a = 10
        print(rs.a) # 10
        print(rs.b) # None
\$\endgroup\$
3
\$\begingroup\$

1. Separation of concerns

There are three main features in this code:

  1. Restricting the keys that can be stored in a dictionary.

  2. Presenting a dictionary as a namespace.

  3. Adding context manager behaviour to shelves in older versions of Python.

The principle of separation of concerns indicates that these should be implemented separately. See §3 below for one way to do this. This makes the code more understandable, testable, and reuseable.

2. Other review points

  1. There are no docstrings. What does this code do? How do I use it?

  2. The code in __exit__ assumes that Shelf objects have context manager behaviour in Python 3.0 or later. But the documentation says that this behaviour was only added in Python 3.4.

  3. Storing _keys as a list means that checking to see if a key is valid takes time proportional to the number of keys. It would be better to store the keys in a set so that checking validity takes constant time.

  4. There are implementations of __getattr__ and __setattr__ but not __delattr__.

  5. A caller who tried to store a key check_key in the shelve would end up overwriting the method of that name.

3. Revised code

  1. Restricting the keys that can be stored in a dictionary.

    import collections.abc
    
    class RestrictedMapping(collections.abc.MutableMapping):
        """A proxy for a mapping that restricts the set of allowable keys.
    
        >>> d = RestrictedMapping({0:0, 1:1}, range(3))
        >>> del d[1]
        >>> d[2] = 2
        >>> d[3] = 3
        Traceback (most recent call last):
            ...
        KeyError: 3
        >>> sorted(d.items())
        [(0, 0), (2, 2)]
    
        """
        def __init__(self, mapping, keys):
            keys = set(keys)
            for key in mapping:
                if key not in keys:
                    raise ValueError('invalid key {}'.format(key))
            self.keys = keys
            self.mapping = mapping
            super(RestrictedMapping, self).__init__()
    
        def _check_key(self, key):
            if key not in self.keys:
                raise KeyError(key)
    
        def __getitem__(self, key):
            self._check_key(key)
            return self.mapping[key]
    
        def __setitem__(self, key, value):
            self._check_key(key)
            self.mapping[key] = value
    
        def __delitem__(self, key):
            self._check_key(key)
            del self.mapping[key]
    
        def __iter__(self):
            return iter(self.mapping)
    
        def __len__(self):
            return len(self.mapping)
    
  2. Presenting a dictionary as a namespace.

    class Namespace(object):
        """A proxy for a mapping that provides access to items via attribute
        lookup.
    
        >>> d = dict(a=1, b=2, c=3)
        >>> n = Namespace(d)        # n is now a proxy for d
        >>> n.d = 4
        >>> n.a, n.b, n.c, n.d
        (1, 2, 3, 4)
        >>> del n.b
        >>> sorted(d.items())
        [('a', 1), ('c', 3), ('d', 4)]
    
        """
        def __init__(self, mapping):
            super(Namespace, self).__setattr__('_mapping', mapping)
    
        def __getattr__(self, attr):
            return self._mapping[attr]
    
        def __setattr__(self, attr, value):
            self._mapping[attr] = value
    
        def __delattr__(self, attr):
            del self._mapping[attr]
    
  3. Combining restriction on keys, presentation as namespace, and addition of context manager behaviour.

    from contextlib import contextmanager
    import shelve
    
    @contextmanager
    def restricted_shelve(filename, keys):
        shelf = shelve.open(filename)
        try:
            yield Namespace(RestrictedMapping(shelf, keys))
        finally:
            shelf.close()
    

4. Namespace antipattern

Presenting a mapping as a namespace instead of a dictionary is superficially attractive because n.name is three characters shorter than d['name'], but it's an antipattern in Python. (The only place this pattern is used in the standard library is in the argparse module.)

The problems are:

  1. Attribute names need to be syntactically valid as Python identifiers, so you have to be very sure you're never going to need an attribute called class or return or 1 or whatever.

  2. There will be collisions between the data attributes and the implementation attributes. In my version in §3.2 above you can't have attributes named _mapping or __init__ or __dict__ or __slots__ or ...

  3. There's no easy way to get at the list of attributes.

  4. There's not much you can do with a namespace-like object: the Python standard library usually expects you to use mappings instead. For example, you can't pass a namespace to itertools.ChainMap, or convert it to JSON using json.dumps, or output it as CSV using csv.DictWriter, and so on.

5. Answers to questions

  1. As far as I know, there's no easy way to implement features 1 (restriction on keys) and 2 (presentation as namespace). Feature 3 (adding context manager behaviour) is more easily done using contextlib.contextmanager as shown in §3.3 above.

  2. Your design puts all features into one class. This technique is known as the "big ball of mud".

  3. Since your code knows exactly when it is assigning a special attribute, it can call super().__setattr__ itself, thus avoiding the need for special handling in the __setattr__ method. See §3.2 above.

  4. Your code looks OK to me, but writing __exit__ methods is complicated and so (if you can) it's better to use contextlib.contextmanager which takes care of this complexity for you.

  5. It's sensible to define your own exception classes, but it's good practice to inherit directly from Exception only if there is no close match in the exception hierarchy. Here UnexpectedKeyError is a runtime problem caused by an invalid value of the correct type, so it makes sense to inherit from ValueError.

    The BadSpecialKey exception indicates a static programming error, not runtime exceptional behaviour, so I would use an assertion here.

  6. It would be better to avoid having special keys altogther: you'll see in §3 above that I didn't need them. But if you must, then @classmethod makes slightly more sense: in theory you might have a subclass with a different value for _SPECIAL_KEYS.

\$\endgroup\$
  • \$\begingroup\$ Thanks, there's a lot to learn here. I see what you mean about the namespace antipattern and will avoid it in the future. However, let me add that namedtuple does function as a namespace. I think that may have been my inspiration; you could describe what I'm trying the accomplish with the code above as either a restricted shelf (as I did) or as a persistent mutable namedtuple. \$\endgroup\$ – kuzzooroo Jun 9 '15 at 14:45
  • \$\begingroup\$ Good point about namedtuple. It looks as if the recordtype package provides a mutable record type, but I don't know whether it serializes easily. \$\endgroup\$ – Gareth Rees Jun 9 '15 at 15:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.