Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use of feature references in the sdk #1032

Merged
merged 5 commits into from
May 21, 2024

Conversation

angaither
Copy link
Contributor

@angaither angaither commented Apr 15, 2024

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:

  • This PR is as small and focused as possible
  • If this PR includes proposed changes for inclusion in the changelog, the title of this PR summarizes those changes and is ready for inclusion in the Changelog.
  • I have updated docstrings for function changes and docs in the 'docs' folder for user interface / behavior changes
  • This PR does not break any examples or I have updated them

(Optional) @mentions for Notifications:

@angaither angaither force-pushed the ag/support-feature-references branch from c071606 to 644a9f4 Compare April 15, 2024 19:42
@angaither angaither force-pushed the ag/support-feature-references branch 6 times, most recently from ce2bf64 to f436273 Compare April 18, 2024 21:18
@angaither angaither force-pushed the ag/support-feature-references branch from f436273 to 8a9c4b6 Compare April 18, 2024 21:24
@angaither angaither changed the title Allow use of feature references in the sdk for subscriptions + orders Allow use of feature references in the sdk Apr 18, 2024
@ischneider
Copy link
Member

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]

@angaither angaither force-pushed the ag/support-feature-references branch 2 times, most recently from d0eaae6 to 818cf67 Compare April 25, 2024 19:54
@angaither angaither force-pushed the ag/support-feature-references branch from 818cf67 to 0b2ce07 Compare April 25, 2024 19:56
@angaither angaither force-pushed the ag/support-feature-references branch 2 times, most recently from e8372a0 to b2650c0 Compare May 20, 2024 17:09
@asonnenschein
Copy link
Contributor

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 angaither force-pushed the ag/support-feature-references branch from b2650c0 to f3bbee9 Compare May 20, 2024 20:03
@angaither
Copy link
Contributor Author

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

@angaither angaither force-pushed the ag/support-feature-references branch 2 times, most recently from 4481acb to 81c3835 Compare May 20, 2024 23:04
@angaither angaither force-pushed the ag/support-feature-references branch 2 times, most recently from 8502879 to 862d92f Compare May 20, 2024 23:13
@angaither angaither merged commit 6c099cb into main May 21, 2024
10 checks passed
@angaither angaither deleted the ag/support-feature-references branch May 21, 2024 15:11
@angaither angaither mentioned this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants