Fix errors reported by mypy #490

sgillies · 2022-05-04T21:19:12Z

Resolves #489

@jreiberkyle @mkshah605 @kevinlacaille this is 6 commits, 9 files. It might look like a long review, but it's only a few lines per file. The TLDR: a few commits fix subtle bugs that our tests were not triggering (static analysis for the win!), the rest of the commits fix improper use of type hints or reconcile our code with mypy's stricter view of typing. I think we should merge this right away if we're going to continue to use type hints in the project. And I think we probably will, users increasingly expect them.

Fixes one mypy-detected error: Type of __all__ must be "Sequence[str]", not "List[object]" Towards resolving #489

Resolves the error Incompatible types in assignment (expression has type "bool", target has type "Collection[Any]")

sgillies · 2022-05-04T21:37:18Z

planet/clients/data.py

+        request_json: typing.Dict[str, typing.Any] = {
            'name': name, 'filter': search_filter, 'item_types': item_types
        }
+        # QUESTION: can we just set the value to True or False above?


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

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?

sgillies · 2022-05-04T21:39:14Z

planet/clients/data.py


        # TODO: validate item_types
-        request_json = {
+        request_json: typing.Dict[str, typing.Any] = {


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.

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?

Added a new AuthType alias for httpx.Auth, added a missing abstract method.

sgillies · 2022-05-04T22:43:13Z

planet/auth.py


+    @abc.abstractmethod
+    def to_dict(self) -> dict:
+        pass


Resolves

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

sgillies · 2022-05-04T22:43:38Z

planet/auth.py

 BASE_URL = f'{PLANET_BASE_URL}/v0/auth'
 ENV_API_KEY = 'PL_API_KEY'

+AuthType = httpx.Auth


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]"

sgillies · 2022-05-04T22:45:35Z

planet/auth.py

        '''
        variable_name = variable_name or ENV_API_KEY
-        api_key = os.getenv(variable_name)
+        api_key = os.getenv(variable_name, '')


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.

This is a nice catch

sgillies · 2022-05-04T23:48:07Z

planet/auth.py

-    def from_login(email: str, password: str, base_url: str = None) -> AuthType:
+    def from_login(email: str,
+                   password: str,
+                   base_url: str = None) -> AuthType:


Carried over formatting.

sgillies · 2022-05-04T23:48:36Z

planet/order_request.py

            order type.
    '''
-    details = {'name': name, 'products': products}
+    details: Dict[str, Any] = {'name': name, 'products': products}


mypy requires us to be more careful about dict typing.

sgillies · 2022-05-04T23:50:09Z

planet/order_request.py

-        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


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

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

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

sgillies · 2022-05-04T23:55:09Z

planet/order_request.py

-        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])


Here, though, mypy's analyzer can't find that anything on the right side will be None, so it doesn't care if we reassign.

And ignore mypy warnings about modules with no typing or stubs.

sgillies · 2022-05-05T00:23:25Z

planet/reporting.py


-        self.bar.refresh()
+        if self.bar is not None:
+            self.bar.refresh()


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

That is cool as heck. I should try mypy

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

sgillies · 2022-05-05T00:30:18Z

@jreiberkyle @mkshah605 @kevinlacaille this is 6 commits, 9 files. It might look like a long review, but it's only a few lines per file. The TLDR: a few commits fix subtle bugs that our tests were not triggering (static analysis for the win!), the rest of the commits fix improper use of type hints or reconcile our code with mypy's stricter view of typing. I think we should merge this right away if we're going to continue to use type hints in the project. And I think we probably will, users increasingly expect them.

sgillies · 2022-05-05T00:31:27Z

planet/__init__.py

+    'order_request',
+    'reporting',
+    'Session',
+]


__all__ requires strings, not objects.

jreiberkyle

its great to have static typing in the python library and turned out to be much easier than I had expected.

jreiberkyle · 2022-05-05T02:56:18Z

planet/auth.py

        '''
        variable_name = variable_name or ENV_API_KEY
-        api_key = os.getenv(variable_name)
+        api_key = os.getenv(variable_name, '')


This is a nice catch

jreiberkyle · 2022-05-05T02:59:45Z

planet/clients/data.py

+        request_json: typing.Dict[str, typing.Any] = {
            'name': name, 'filter': search_filter, 'item_types': item_types
        }
+        # QUESTION: can we just set the value to True or False above?


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?

planet/clients/data.py

jreiberkyle · 2022-05-05T03:03:26Z

planet/clients/data.py


        # TODO: validate item_types
-        request_json = {
+        request_json: typing.Dict[str, typing.Any] = {


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?

jreiberkyle · 2022-05-05T03:07:09Z

planet/order_request.py

-        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


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

jreiberkyle · 2022-05-05T03:08:02Z

planet/reporting.py


-        self.bar.refresh()
+        if self.bar is not None:
+            self.bar.refresh()


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

Removes a conditional statement and eliminates one of the original mypy errors.

jreiberkyle · 2022-05-05T18:20:02Z

@sgillies can you please also add how to use mypy into CONTRIBUTING.md?

sgillies · 2022-05-05T18:36:43Z

Done in ff57b41.

sgillies added 3 commits May 4, 2022 15:02

Strings, not objects

3b1e53f

Fixes one mypy-detected error: Type of __all__ must be "Sequence[str]", not "List[object]" Towards resolving #489

Fix __all__ in another module, use single quotes

2f6ec47

Add typing to request dict

2efe536

Resolves the error Incompatible types in assignment (expression has type "bool", target has type "Collection[Any]")

sgillies commented May 4, 2022

View reviewed changes

Our auth.Auth isn't type-compatible with httpx.AsyncClient

2e81249

Added a new AuthType alias for httpx.Auth, added a missing abstract method.

sgillies commented May 4, 2022

View reviewed changes

Fix mypy errors in order_requests

278ecd7

sgillies commented May 4, 2022

View reviewed changes

Strictly guard against having no status bar in reporting.py

fa4a9b1

And ignore mypy warnings about modules with no typing or stubs.

sgillies commented May 5, 2022

View reviewed changes

sgillies marked this pull request as ready for review May 5, 2022 00:24

sgillies requested review from jreiberkyle, kevinlacaille and mkshah605 May 5, 2022 00:24

sgillies added this to the Data CLI MVP milestone May 5, 2022

sgillies self-assigned this May 5, 2022

sgillies added the fixup label May 5, 2022

sgillies commented May 5, 2022

View reviewed changes

sgillies added the maintenance label May 5, 2022

jreiberkyle approved these changes May 5, 2022

View reviewed changes

Always set email flag in search request

0d187df

Removes a conditional statement and eliminates one of the original mypy errors.

sgillies mentioned this pull request May 5, 2022

Use mypy to check type hints #489

Closed

sgillies merged commit 72eb746 into main May 5, 2022

sgillies deleted the issue489 branch May 5, 2022 14:44

Fix errors reported by mypy #490

Fix errors reported by mypy #490

Uh oh!

Conversation

sgillies commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgillies May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgillies May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgillies commented May 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreiberkyle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreiberkyle commented May 5, 2022

Uh oh!

sgillies commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sgillies commented May 4, 2022 •

edited

Loading

sgillies May 4, 2022 •

edited

Loading

sgillies May 4, 2022 •

edited

Loading