-
Notifications
You must be signed in to change notification settings - Fork 98
Move placeholder client to new client/subscriptions.py module. #588
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
Conversation
planet/cli/orders.py
Outdated
| '''Commands for interacting with the Orders API''' | ||
| ctx.obj['AUTH'] = planet.Auth.from_file() | ||
| # ctx.obj['AUTH'] = planet.Auth.from_file() | ||
| ctx.obj['AUTH'] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to get credentials from an environment variable. See also #589.
| @@ -0,0 +1,171 @@ | |||
| """Planet Subscriptions API Python client.""" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code of this module was moved here from planet/cli/subscriptions.py with no other changes.
| """Prints the expected sequence of subscriptions.""" | ||
|
|
||
| monkeypatch.setattr(planet.cli.subscriptions, | ||
| monkeypatch.setattr(planet.clients.subscriptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_fake_subs has been moved.
That's the pattern in orders and data but it's not strictly needed. One line of code only is added to commands and the need to document and test another function is eliminated.
| """Gets results of a subscription and prints the API response.""" | ||
| async with subscriptions_client(ctx) as client: | ||
| async with Session(auth=ctx.obj['AUTH']) as session: | ||
| client = SubscriptionsClient(session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing our usage pattern adds 6 lines of code (one per command) and removes 12 lines of code (the subscriptions_client, its requirements, and docs).
| filtered_results = client.list_subscription_results(subscription_id, | ||
| status=status, | ||
| limit=limit) | ||
| filtered_results = client.get_results(subscription_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the operationId in https://api.planet.com/subscriptions/v1/spec. iter_results would be better from a Python programmer perspective, but alignment with the spec has benefits too.
| method provides iteration over subcriptions, it does not return | ||
| a list. | ||
| Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to follow Google's docstring standard we should use "Args" instead of "Parameters".
| """ | ||
| # Temporary marker for behavior of module with unpatched state. | ||
| if _fake_subs is None: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to say that the placeholder client doesn't do anything except in the context of the CLI tests.
| '''Commands for interacting with the Orders API''' | ||
| # ctx.obj['AUTH'] = planet.Auth.from_file() | ||
| ctx.obj['AUTH'] = None | ||
| ctx.obj['AUTH'] = planet.Auth.from_file() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back, this can be addressed separately in #589.
Also a fix for #574
Resolves #494