Add a clip_to_source kwarg to subscript_request.build_request #971
Conversation
This is a preview of the behavior of the next API version.
planet/cli/subscriptions.py
Outdated
| @click.option( | ||
| '--tools', | ||
| type=types.JSON(), | ||
| help='Toolchain JSON. Can be a string, filename, or - for stdin.') |
There was a problem hiding this comment.
This is all on yapf and not explained by anything in its change log 😒
There was a problem hiding this comment.
And it's version dependent. With Python 3.7, yapf 0.40.1 puts '--tools' on line 208. WTH.
There was a problem hiding this comment.
We're now using Python 3.8 for the CI linting checks.
planet/subscription_request.py
Outdated
| exists. NOTE: the next version of the Subscription API will | ||
| remove the clip tool option and always clip to the source | ||
| geometry. Thus this is a preview of the next API version's | ||
| default behavior. |
There was a problem hiding this comment.
Moving this above the example.
| delivery: dict, | ||
| notifications: Optional[dict] = None, | ||
| tools: Optional[List[dict]] = None) -> dict: | ||
| source: Mapping, |
There was a problem hiding this comment.
Typing best practices (https://typing.readthedocs.io/en/latest/source/best_practices.html) is to accept mappings (most general) and return dicts (most specific). This follows Postel's Law.
| """ | ||
| details = {"name": name, "source": source, "delivery": delivery} | ||
| details = { | ||
| "name": name, "source": dict(source), "delivery": dict(delivery) |
There was a problem hiding this comment.
Here is the consequence of accepting mappings: we need to explicitly convert them to dicts here. The upside is that we will never modify dicts that users pass in to this function, which can cause subtle bugs.
There was a problem hiding this comment.
We copy information now, which has some overhead, but makes calling code more reliable. And building a subscriptions request doesn't need to be super optimized.
This is a preview of the behavior of the next API version.
Also temporarily pin click to < 8.1.4
This could be a preview of the behavior of the next API version.
Resolves #967