stac-utils / pystac

Python library for working with any SpatioTemporal Asset Catalog (STAC)
https://pystac.readthedocs.io
Other
346 stars 116 forks source link

`CatalogExt.has()` throws KeyError on unknown extension #1350

Closed soxofaan closed 2 months ago

soxofaan commented 3 months ago
import pystac

c = pystac.Collection(id="foo", description="bar", extent=None)
c.ext.has("meh")

throws

KeyError: "Extension 'meh' is not a valid extension. Options are ['classification', 'cube', 'eo', 'file', 'grid', 'item_assets', 'mgrs', 'pc', 'proj', 'raster', 'sar', 'sat', 'sci', 'storage', 'table', 'timestamps', 'version', 'view', 'xarray']"

Is it the intended design to raise an exception here? As a user, I expect has() to return false if the specified extension is not available for consumption.

jsignell commented 3 months ago

I think that behavior is intentional but open to suggestions on how it could be improved. That error is meant to say: there is no extension named "meh" this is as opposed to the False condition which would indicate that the extension in question has not been added to the specified collection.

soxofaan commented 3 months ago

To give some context about my expected behavior: As a user or even more strongly as a library developer I want to build on pystac and write something like

if collection.ext.has("foo"):
    # do something with "foo" extension related metadata

Without worrying about whether/how "foo" is defined as an extension in some other place. Now I have to wrap that in try-catch boilerplate

Or is there a cleaner way to first dynamically check if the "foo" extension exists? Something like

if pystac.ext.has("foo") and collection.ext.has("foo"):
     # ...

But then again, from user standpoint this looks a bit redundant and boilerplaty

gadomski commented 3 months ago

We don't currently have a graceful mechanism to register third party extensions into the ext attribute of catalog/collection/item/asset, so pystac.extensions.ext.EXTENSION_NAMES list is the exhaustive list of supported extensions — unless you're doing something wild, that list should be fixed for your current pystac version.

Or is there a cleaner way to first dynamically check if the "foo" extension exists?

Yup:

from pystac.extensions.ext import EXTENSION_NAMES
if "foo" in EXTENSION_NAMES and collection.ext.has("foo"):
    ...
soxofaan commented 3 months ago

EXTENSION_NAMES is apparently a typing.Literal object, and on Python 3.9 (which we still want to support) that does not work:

from pystac.extensions.ext import EXTENSION_NAMES
"foo" in EXTENSION_NAMES

gives

TypeError: Parameters to generic types must be types. Got 0.

(It only seems to work on python 3.11 or higher)

gadomski commented 3 months ago

Ah gotcha ... For <= 3.10 you could probably do this instead:

from pystac.extensions.ext import EXTENSION_NAMES_MAPPING
"foo" in EXTENSION_NAMES_MAPPING
jsignell commented 3 months ago

I am fine with changing the behavior of has if that makes more sense. Do you think that has is clearer than just using hasattr though?

if hasattr(c.ext, "meh"):
    ...
gadomski commented 3 months ago

I am fine with changing the behavior of has if that makes more sense.

I don't think it does — IMO "does this thing implement this known extension" needs a different behavior than "we don't know what that extension is".

jsignell commented 2 months ago

I'm going to go ahead and close this, but @soxofaan if you think the error message could be clearer please open a pull request.