planetlabs / planet-client-python

Python client for Planet APIs
https://planet-sdk-for-python-v2.readthedocs.io/en/latest/
Apache License 2.0
274 stars 92 forks source link

Allow use of feature references in the sdk #1032

Closed angaither closed 5 months ago

angaither commented 6 months ago

Related Issue(s):

Closes #

Proposed Changes:

For inclusion in changelog (if applicable):

  1. Allow feature references to be used in the sdk for creating subscriptions (with or without a source block) and for orders.
  2. Create a top level geometry filter in the data api, allow references and features there

Not intended for changelog:

Diff of User Interface

Old behavior: geojson was required to make a subscription or order.

New behavior: geojson or a geometry feature reference can be used

{
        "type": "ref",
        "content": "pl:features/my/water-fields-RqB0NZ5/rmQEGqm",
} 

Or in the cli a ref can be used as a string

 planet data search PSScene --geom 'pl:features/my/water-fields-RqB0NZ5/rmQEGqm' | jq

PR Checklist:

(Optional) @mentions for Notifications:

ischneider commented 6 months ago

There's a number of geometry: Optional[dict] though the typing intention is now including str (ref)

I haven't dug enough to see if it's even possible (or useful) to try to specify a GeoJSON type (like by applying more type variables to dict keys, though honestly it doesn't feel useful)...

But something like this might be a nice compromise (untested in mypy or other static checker)

from typing import Union

# GeoJSON is a python dict that conforms to the specification
GeoJSON = dict

# GeoRef is a reference to a feature, either as a file or URI, that if resolved, would yield GeoJSON
GeomRef = str

# Geom is some inline or reference to a Geometry
Geom = Union[GeoJSON, GeomRef]
asonnenschein commented 5 months ago

I see this MR includes integration tests for the Data API/CLI, and the Subscriptions CLI. @angaither Do you think we also need tests for the Subscriptions API and Orders API/CLI?

angaither commented 5 months ago

I see this MR includes integration tests for the Data API/CLI, and the Subscriptions CLI. @angaither Do you think we also need tests for the Subscriptions API and Orders API/CLI?

We probably only need CLI tests since that is doing a bit more manipulation and allowing strings, so I added a test for the orders CLI. The API tests probably aren't needed since

  1. we are just passing values through to the respective APIs and this feature is already tested in those codebases
  2. They're mocked clients