-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add planet.sync Namespace #1078
Conversation
@@ -29,7 +27,6 @@ | |||
'OrdersClient', | |||
'order_request', | |||
'reporting', | |||
'Planet', |
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.
What if we kept Planet
exported at the top-level (e.g. from planet import Planet
and only move the individual API classes to planet.sync
? Thinking through this a little bit more since the discussion yesterday, from planet.sync import Planet
could be harder for users to discover without referencing the docs, and sync
is probably not as meaningful to end users as it is for us.
from planet import *
may be cluttered but I think it's appropriate for Planet/AsyncPlanet (our entrypoints) to get first pick at this namespace.
Otherwise this looks like a good approach to address the docs issue - just thinking out loud about the best way for users to discover how to start using the SDK.
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.
from planet import * may be cluttered but I think it's appropriate for Planet/AsyncPlanet (our entrypoints) to get first pick at this namespace.
Good idea @stephenhillier, I updated this PR to promote the sync Planet class wrapper to the top level namespace. So it can be imported via from planet import Planet
, and from planet import *
. I still have the sync Planet class wrapper living in ./sync/client.py
.
cbaaf7b
to
f9513c2
Compare
3ec025c
to
65a933a
Compare
f9513c2
to
6da8316
Compare
planet/sync/__init__.py
Outdated
|
||
__all__ = ['DataAPI', 'OrdersAPI', 'Planet', 'SubscriptionsAPI'] | ||
|
||
# Organize client classes by their module name to allow lookup. |
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.
I don't think we need/want this ... this is a relic of a bizarre pattern that I believe came from dealing w/ circular deps
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.
Understood, thanks for shedding some light on this pattern for me! I have never seen it before in a Python project, and added it here because I saw it elsewhere in this project. Removed.
planet/sync/__init__.py
Outdated
from .subscriptions import SubscriptionsAPI | ||
from .client import Planet | ||
|
||
__all__ = ['DataAPI', 'OrdersAPI', 'Planet', 'SubscriptionsAPI'] |
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.
Nit: the whole idea of Planet is that the user no longer has to consider the other APIs... If they want to, they can import them individually but I think it's confusing the import the "shortcut" (Planet) as well as the thing it shortcuts to...
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.
Good idea to keep things simple. I removed the API clients from the all namespace and just have the Planet
wrapper.
planet/sync/client.py
Outdated
@@ -34,7 +34,8 @@ class Planet: | |||
The session can be used to control the authentication method. Example: | |||
|
|||
```python | |||
from planet import Auth, Session, Planet | |||
from planet import Auth, Session | |||
from planet.sync import Planet | |||
|
|||
auth = Auth.from_key('examplekey') |
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.
I know this is existing doc but maybe we should fix the example so folks don't get the idea they should hard-code their key into source code... e.g.
auth = Auth.from_key(get_key_from_encrypted_file())
or better, just note the Session can be provided allowing for whatever customization but defaults to standard behavior...
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.
We definitely don't want folks thinking they should hard-code their keys 😅
I went ahead and updated the language here, with a code example that uses the default session.
This PR is branched off of #1049. It takes the new sync DataAPI, Subscriptions API, and OrdersAPI clients, and Planet wrapper class, and puts them in a new
planet.sync
namespace. The original async clients still live in their original namespace to support backwards compatibility. While I am open to the idea of adding an additional namespace,planet.async
for the async clients, I did not include that in this PR because we need to support backwards compatibility for the existing async client patterns and addingplanet.async
would then be a third namespace which adds more complexity through maintenance overhead.Examples: