Skip to content

Fix errors reported by mypy #490

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 5, 2022
Merged

Fix errors reported by mypy #490

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3b1e53f
Strings, not objects
sgillies May 4, 2022
2f6ec47
Fix __all__ in another module, use single quotes
sgillies May 4, 2022
2efe536
Add typing to request dict
sgillies May 4, 2022
2e81249
Our auth.Auth isn't type-compatible with httpx.AsyncClient
sgillies May 4, 2022
278ecd7
Fix mypy errors in order_requests
sgillies May 4, 2022
fa4a9b1
Strictly guard against having no status bar in reporting.py
sgillies May 5, 2022
0d187df
Always set email flag in search request
sgillies May 5, 2022
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
View file
Open in desktop
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", "planet")


@nox.session
def coverage(session):
session.install("-e", ".[test]")
Expand Down
9 changes: 8 additions & 1 deletion planet/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,11 @@
from .auth import Auth
from .clients import DataClient, OrdersClient

__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.

4 changes: 2 additions & 2 deletions planet/clients/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
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