crim-ca / stac-populator

Workflow logic to populate STAC catalog with demo datasets.
MIT License
2 stars 2 forks source link

pystac extension not handling well `name` property #63

Open huard opened 3 weeks ago

huard commented 3 weeks ago

The CMIP6Extension has a name property:

    @property
    def name(self) -> SchemaName:
        return get_args(SchemaName)[0]

but this is not handled well by pystac.extensions: https://github.com/stac-utils/pystac/blob/ba406cb7991f992ac6f38026aa779adcc6a4f67a/pystac/extensions/base.py#L258

where getattr(cls, "name") returns a property object, not the actual name.

fmigneault commented 3 weeks ago

Can you try replacing by the following and see if that works?

    @classmethod
    def name(cls) -> SchemaName:
        return get_args(SchemaName)[0]
huard commented 3 weeks ago

That would return a <bound method>.

mishaschwartz commented 3 weeks ago

geattr(cls, 'name') will return the object defined in the given class that is named name. In this case that is a function. We want the return value of the function, not the function itself. Since the body of the name function does not depend on anything specific to the class itself, we can just refactor the code so name isn't a function anymore and is a class variable instead:

class CMIP6Extension:
    name = get_args(SchemaName)[0]
    ...

minimal example:

from typing import get_args, Literal

SchemaName = Literal["cmip6"]

class CMIP6Extension:
    name = get_args(SchemaName)[0]
    ...

getattr(CMIP6Extension, "name") # returns "cmip6"

Note that if the annotation matters for pydantic we may need to annotate the class variable as well:

name: SchemaName = get_args(SchemaName)[0]
huard commented 3 weeks ago

The class where it appears is not a pydantic BaseModel, so a simple class attribute will work.

fmigneault commented 3 weeks ago

Could use pystac's own approach: https://github.com/stac-utils/pystac/blob/ba406cb7991f992ac6f38026aa779adcc6a4f67a/pystac/extensions/classification.py#L174-L181

Otherwise, the simple class attribute as mentioned by @mishaschwartz also works.