Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,18 @@
nox.options.stop_on_first_error = True
nox.options.reuse_existing_virtualenvs = False

nox.options.sessions = ['lint', 'test', 'coverage', 'docs']
nox.options.sessions = ['lint', 'analyze', 'test', 'coverage', 'docs']

source_files = ("planet", "examples", "tests", "setup.py", "noxfile.py")


@nox.session
def analyze(session):
session.install(".[lint]")

session.run("mypy", "--ignore-missing", "planet")


@nox.session
def coverage(session):
session.install("-e", ".[test]")
Expand Down
11 changes: 9 additions & 2 deletions planet/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@
from . import order_request, reporting
from .__version__ import __version__ # NOQA
from .auth import Auth
from .clients import DataClient, OrdersClient
from .clients import DataClient, OrdersClient # NOQA

__all__ = [Session, OrdersClient, order_request, reporting, Auth, DataClient]
__all__ = [
'Auth',
'DataClient'
'OrdersClient',
'order_request',
'reporting',
'Session',
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__all__ requires strings, not objects.

20 changes: 14 additions & 6 deletions planet/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@
BASE_URL = f'{PLANET_BASE_URL}/v0/auth'
ENV_API_KEY = 'PL_API_KEY'

AuthType = httpx.Auth
Copy link
Contributor Author

@sgillies sgillies May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves

planet/http.py:123: error: Argument "auth" to "AsyncClient" has incompatible type "planet.auth.Auth"; expected "Union[Tuple[Union[str, bytes], Union[str, bytes]], Callable[[Request], Request], httpx._auth.Auth, None]"



class Auth(metaclass=abc.ABCMeta):
'''Handle authentication information for use with Planet APIs.'''

@staticmethod
def from_key(key: str) -> Auth:
def from_key(key: str) -> AuthType:
'''Obtain authentication from api key.

Parameters:
Expand All @@ -46,7 +48,7 @@ def from_key(key: str) -> Auth:
return auth

@staticmethod
def from_file(filename: str = None) -> Auth:
def from_file(filename: str = None) -> AuthType:
'''Create authentication from secret file.

The secret file is named `.planet.json` and is stored in the user
Expand All @@ -71,7 +73,7 @@ def from_file(filename: str = None) -> Auth:
return auth

@staticmethod
def from_env(variable_name: str = None) -> Auth:
def from_env(variable_name: str = None) -> AuthType:
'''Create authentication from environment variable.

Reads the `PL_API_KEY` environment variable
Expand All @@ -80,7 +82,7 @@ def from_env(variable_name: str = None) -> Auth:
variable_name: Alternate environment variable.
'''
variable_name = variable_name or ENV_API_KEY
api_key = os.getenv(variable_name)
api_key = os.getenv(variable_name, '')
Copy link
Contributor Author

@sgillies sgillies May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves

planet/auth.py:85: error: Argument 1 to "APIKeyAuth" has incompatible type "Optional[str]"; expected "str"

The APIKeyAuth constructor does not take None as an argument (key: str) but we might pass it None if there's nothing in the environment. Instead, we'll pass an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice catch

try:
auth = APIKeyAuth(api_key)
LOGGER.debug(f'Auth set from environment variable {variable_name}')
Expand All @@ -91,7 +93,9 @@ def from_env(variable_name: str = None) -> Auth:
return auth

@staticmethod
def from_login(email: str, password: str, base_url: str = None) -> Auth:
def from_login(email: str,
password: str,
base_url: str = None) -> AuthType:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carried over formatting.

'''Create authentication from login email and password.

Note: To keep your password secure, the use of `getpass` is
Expand All @@ -113,14 +117,18 @@ def from_login(email: str, password: str, base_url: str = None) -> Auth:

@classmethod
@abc.abstractmethod
def from_dict(cls, data: dict) -> Auth:
def from_dict(cls, data: dict) -> AuthType:
pass

@property
@abc.abstractmethod
def value(self):
pass

@abc.abstractmethod
def to_dict(self) -> dict:
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves

planet/auth.py:132: error: "Auth" has no attribute "to_dict"


def write(self, filename: str = None):
'''Write authentication information.

Expand Down
4 changes: 2 additions & 2 deletions planet/clients/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@
from .orders import OrdersClient

__all__ = [
DataClient,
OrdersClient,
'DataClient',
'OrdersClient',
]
5 changes: 3 additions & 2 deletions planet/clients/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,12 @@ async def create_search(self,
url = self._searches_url()

# TODO: validate item_types
request_json = {
request_json: typing.Dict[str, typing.Any] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this hint, mypy diagnoses request_json's type as Dict[str, Collection[Any]] because name, search_filter, and item_types are all collections, and then complains when we try to add a bool item, which is not a collection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh interesting! good catch. I wonder if we want to make this a custom type, eg JSONType and move it over, along with AuthType to a new types module?

'name': name, 'filter': search_filter, 'item_types': item_types
}
# QUESTION: can we just set the value to True or False above?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a test that doesn't expect __daily_email_enabled: False, but I wonder if the API would handle this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API reference claims that the type of this parameter is a boolean so I imagine it would handle True/False values. Is that the question here?

if enable_email:
request_json['__daily_email_enabled'] = True
request_json['__daily_email_enabled'] = enable_email

request = self._request(url, method='POST', json=request_json)
response = await self._do_request(request)
Expand Down
4 changes: 2 additions & 2 deletions planet/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import httpx

from .auth import Auth
from .auth import Auth, AuthType
from . import exceptions, models
from .__version__ import __version__

Expand Down Expand Up @@ -112,7 +112,7 @@ class Session(BaseSession):
```
'''

def __init__(self, auth: Auth = None):
def __init__(self, auth: AuthType = None):
"""Initialize a Session.

Parameters:
Expand Down
21 changes: 11 additions & 10 deletions planet/order_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"""Functionality for preparing order details for use in creating an order"""
from __future__ import annotations # https://stackoverflow.com/a/33533514
import logging
from typing import List
from typing import Any, Dict, List

from . import geojson, specs

Expand Down Expand Up @@ -67,7 +67,7 @@ def build_request(name: str,
planet.specs.SpecificationException: If order_type is not a valid
order type.
'''
details = {'name': name, 'products': products}
details: Dict[str, Any] = {'name': name, 'products': products}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy requires us to be more careful about dict typing.


if subscription_id:
details['subscription_id'] = subscription_id
Expand All @@ -79,8 +79,8 @@ def build_request(name: str,
details['notifications'] = notifications

if order_type:
order_type = specs.validate_order_type(order_type)
details['order_type'] = order_type
validated_order_type = specs.validate_order_type(order_type)
details['order_type'] = validated_order_type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, mypy discourages reassigning names, and not without reason because it can be a source of bugs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! yeah, I can see that. nice catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reassign names a lot and it's a hard habit to break 😆


if tools:
details['tools'] = tools
Expand Down Expand Up @@ -108,18 +108,19 @@ def product(item_ids: List[str],
are not valid bundles or if item_type is not valid for the given
bundle or fallback bundle.
'''
product_bundle = specs.validate_bundle(product_bundle)
item_type = specs.validate_item_type(item_type, product_bundle)
validated_product_bundle = specs.validate_bundle(product_bundle)
item_type = specs.validate_item_type(item_type, validated_product_bundle)

if fallback_bundle is not None:
fallback_bundle = specs.validate_bundle(fallback_bundle)
specs.validate_item_type(item_type, fallback_bundle)
product_bundle = ','.join([product_bundle, fallback_bundle])
validated_fallback_bundle = specs.validate_bundle(fallback_bundle)
specs.validate_item_type(item_type, validated_fallback_bundle)
validated_product_bundle = ','.join(
[validated_product_bundle, validated_fallback_bundle])

product_dict = {
'item_ids': item_ids,
'item_type': item_type,
'product_bundle': product_bundle
'product_bundle': validated_product_bundle
}
return product_dict

Expand Down
20 changes: 12 additions & 8 deletions planet/reporting.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,19 @@ def update_state(self, state: str):
def update(self, state: str = None, order_id: str = None):
if state:
self.state = state
try:
self.bar.postfix[1] = self.state
except AttributeError:
# If the bar is disabled, attempting to access self.bar.postfix
# will result in an error. In this case, just skip it.
pass
if self.bar is not None:
try:
self.bar.postfix[1] = self.state
except AttributeError:
# If the bar is disabled, attempting to access
# self.bar.postfix will result in an error. In this
# case, just skip it.
pass

if order_id:
self.order_id = order_id
self.bar.set_description_str(self.desc, refresh=False)
if self.bar is not None:
self.bar.set_description_str(self.desc, refresh=False)

self.bar.refresh()
if self.bar is not None:
self.bar.refresh()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy noticed some cases where self.bar could be None and we would try to call its methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is cool as heck. I should try mypy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome catch. this would probably have bubbled up as a bug eventually.

2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
'pytest', 'pytest-asyncio==0.16', 'pytest-cov', 'respx==0.16.3'
]

lint_requires = ['flake8', 'yapf']
lint_requires = ['flake8', 'mypy', 'yapf']

doc_requires = [
'mkdocs==1.3',
Expand Down