Use descriptors for Extensions?

[TomAugspurger]

I'm writing up an extension, and my wrists hurt from all the typing :) I wonder if we could use descriptors to cut down on the boilerplate. For a typical property that comes from the properties dict, it'd be something like

class MyExtension:
    description = Property()

where Property is a descriptor like

# untested
class Property:
    def __set_name__(self, owner, name):
        self.name= name

    def __get__(self, obj, objtype=None):
        return getattr(obj, "properties")[self.name)
    def __set__(self, obj, value):
        getattr(obj, "properties")[self.name] = value

I haven't really looked to see if this works for many of the extensions. Some downsides:

1. Unclear how this would interact with static typing. But properties are supported by mypy, and descriptors are prosperities, so it might be doable.
2. Makes . lookup a bit slower, though on par with @property so maybe it's acceptable.

[duckontheweb]

2. Makes . lookup a bit slower, though on par with @property so maybe it's acceptable.

It sounds like Python uses descriptors under the hood for @property, so we would only see a performance hit when we replace true attributes with this pattern, which isn't very often.

I haven't really looked to see if this works for many of the extensions.

I haven't tested this, but it seems like it would be a good pattern in the extensions. Those properties are doing a dynamic lookup on properties of the underlying item anyway, and that seems like a good use-case for descriptors.

I'm definitely +1 on this suggestion in theory, but I think it would be helpful to see an example of how it plays out in one of the simpler classes like pystac.Asset or pystac.Link to determine whether the change is worth it. In addition to the question around typing we would also want to make sure that we can get docstrings to come through properly for the API docs.

[TomAugspurger]

There's a slight inconsistency with how datacube handles setting None for a value, and I wanted to check that it's deliberate. For required fields, setting the value to None just sets the value in properties. For optional fields, setting the value to None deletes the field from properties

>>> ext = DatacubeExtension(...)
>>> dim = ext.dimensions["x"]
>>> dim.dim_type = None
>>> "type" in dim.properties
True
>>> dim.description = None
>>> "description" in dim.properties
False

If that's deliberate, then I think that the following will work.

class Property:
    def __init__(self, key: str, obj: Union[str, Any]=None, required=False):
        self.key = key
        self.obj = obj
        self.required = required

    def __get__(self, obj, objtype=None):
        if self.required:
            return get_required(
                getattr(obj, "properties").get(self.key),
                self.obj,
                self.key
            )
        else:
            return self.properties.get(self.key)

    def __set__(self, obj, value):
        properties = getattr(obj, "properties")
        if value is None and not self.required:
            properties.pop(self.key, None)
        else:
            properties[self.key] = value

usage

    properties: Dict[str, Any]
    dim_type = Property(DIM_TYPE_PROP, "cube:dimensions", required=True)
    description = Property(DIM_DESC_PROP)

I'm updating the datacube extension for stac-extensions/datacube#6, and I'd slightly prefer to have this descriptor in place first, so I think I'll prioritize that as a precursor PR :)

Edit: oh, there's the issue around static typing though. I'll play with it for 10 minutes, and if I can't figure things out I'll just make the updates to Datacube using the old style.

[TomAugspurger]

I cheated and took longer than 10 minutes, but python/mypy#2566 (comment) was helpful. This gets the type correct I think:

U = TypeVar("U")


class Property(Generic[U]):
    def __init__(self, key: str, obj: Union[str, Any]=None, required=False):
        self.key = key
        self.obj = obj
        self.required = required

    @overload
    def __get__(self, instance: None, owner: Any) -> "Property": ...

    @overload
    def __get__(self, instance: object, owner: Any) -> U: ...

    def __get__(self, obj, objtype=None) -> Union["Property", U]:
        if self.required:
            return get_required(
                getattr(obj, "properties").get(self.key),
                self.obj,
                self.key
            )
        else:
            return self.properties.get(self.key)

    def __set__(self, obj, value):
        properties = getattr(obj, "properties")
        if value is None and not self.required:
            properties.pop(self.key, None)
        else:
            properties[self.key] = value

I updated the usage to include the type annotation.

dim_type: Property[str] = Property(DIM_TYPE_PROP, "cube:dimensions", required=True)

Test file

# file: tst.py
import pystac
from typing import cast

item = pystac.read_file("tests/data-files/datacube/item.json")
# just doing item: pystac.Item gives:
# tst.py:4: error: Value of type variable "T" of "ext" of "DatacubeExtension" cannot be "STACObject"  [type-var]
item = cast(pystac.Item, item)

ext = pystac.extensions.datacube.DatacubeExtension.ext(item)

reveal_type(ext)
dim = ext.dimensions["x"]

reveal_type(dim)

reveal_type(dim.dim_type)

Running mypy

$ mypy tst.py
tst.py:11: note: Revealed type is "pystac.extensions.datacube.DatacubeExtension[pystac.item.Item*]"
tst.py:14: note: Revealed type is "pystac.extensions.datacube.Dimension*"
tst.py:16: note: Revealed type is "builtins.str*"

which matches the behavior on main.