health: move to mypy #1130

WilliamBergamin · 2024-08-15T21:43:07Z

MyPy was found to execute much fast the PyType in this project, these changes adapt the project to use MyPy instead of PyType

pytype	mypy

11m27s	43s

From my short experience with mypy:

Good documentation 📜
Good error surfacing
Easy to configure
It offers a language server, this helps with development since it integrate with IDEs

Concern: This tool can be strict if we let it, I think it will lead to "safer", more resilient, code. We should not let it get in the way of writing clear and efficient code.

Note: removing all the type: ignore comments from the project may cause breaking changes or have a ripple effect through the entire project

Let me know If we want more tests to cover these changes 🤔

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run ./scripts/install_all_and_run_tests.sh after making the changes.

codecov · 2024-08-15T21:47:37Z

Codecov Report

Attention: Patch coverage is 91.47059% with 29 lines in your changes missing coverage. Please review.

Project coverage is 92.01%. Comparing base (ccbb541) to head (c002770).
Report is 1 commits behind head on main.

Files	Patch %	Lines
slack_bolt/authorization/async_authorize.py	54.54%	5 Missing ⚠️
slack_bolt/authorization/authorize.py	54.54%	5 Missing ⚠️
slack_bolt/adapter/wsgi/handler.py	50.00%	3 Missing ⚠️
slack_bolt/adapter/asgi/base_handler.py	66.66%	2 Missing ⚠️
slack_bolt/adapter/asgi/aiohttp/__init__.py	50.00%	1 Missing ⚠️
slack_bolt/adapter/asgi/builtin/__init__.py	66.66%	1 Missing ⚠️
slack_bolt/adapter/socket_mode/aiohttp/__init__.py	87.50%	1 Missing ⚠️
...ck_bolt/adapter/socket_mode/websockets/__init__.py	87.50%	1 Missing ⚠️
slack_bolt/app/app.py	94.44%	1 Missing ⚠️
slack_bolt/app/async_app.py	94.73%	1 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1130      +/-   ##
==========================================
+ Coverage   92.00%   92.01%   +0.01%     
==========================================
  Files         195      195              
  Lines        6606     6616      +10     
==========================================
+ Hits         6078     6088      +10     
  Misses        528      528

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

WilliamBergamin · 2024-08-16T15:24:19Z

slack_bolt/app/app.py

@@ -108,7 +109,7 @@ def __init__(
        user_facing_authorize_error_message: Optional[str] = None,
        installation_store: Optional[InstallationStore] = None,
        # for either only bot scope usage or v1.0.x compatibility
-        installation_store_bot_only: Optional[bool] = None,
+        installation_store_bot_only: bool = False,


This change is my biggest concern, I'm not sure why this parameter was optional it is never used as a None value always as a boolean

Let me know I should revert this to keep it nullable

Seems odd to have an optional boolean, unless the None state has a special meaning and so installation_store_bot_only is meant to have one of three states?

@WilliamBergamin
This is intentional. When it's None, it means "unset" for the top-level parameter. L310 checks if it's explicitly set or not, and if it's set, then it compares the set value with "oauth_flow.settings.installation_store_bot_only." Developers could set both, thus implementing it this way to prioritize the top-level "installation_store_bot_only" if both are intentionally set. Don't change this logic. Adding some comments should be helpful for future maintainers!

I see I've reverted these changes 🧠

WilliamBergamin · 2024-08-16T15:28:08Z

slack_bolt/app/app.py

@@ -191,7 +192,8 @@ def message_hello(message, say):
            listener_executor: Custom executor to run background tasks. If absent, the default `ThreadPoolExecutor` will
                be used.
        """
-        signing_secret = signing_secret or os.environ.get("SLACK_SIGNING_SECRET", "")
+        if signing_secret is None:


This change allows mypy to properly narrow the type of signing_secret from Optional[str] to str

It sounds os.environ.get(name, default_value)'s typing is not smart enough but this makes sense

WilliamBergamin · 2024-08-16T15:30:05Z

slack_bolt/app/app.py

@@ -413,7 +417,7 @@ def _init_middleware_list(
                )
            else:
                raise BoltError(error_token_required())
-        else:
+        elif self._authorize is not None:


self._authorize should never be None in the following logic, this change enforces the behavior

Good catch (by mypy); if we change this branching to elif, having else clause, which raises an exception, would be a plus

Great suggestion, I've added a BoltError exception, but I'm unsure of its message

else: raise BoltError("OAuthFlow or Authorize must be configured to make a Bolt app")

Let me know if this is sufficient 🤔

The error message looks good to me. For consistency, move the string to slack_bolt.logger.messages like other parts in this class do!

slack_bolt/authorization/async_authorize.py

slack_bolt/listener/async_listener_error_handler.py

WilliamBergamin · 2024-08-16T17:09:34Z

slack_bolt/middleware/authorization/async_multi_teams_authorization.py

@@ -96,7 +96,7 @@ async def async_process(
                    "Although the app should be installed into this workspace, "
                    "the AuthorizeResult (returned value from authorize) for it was not found."
                )
-                if req.context.response_url is not None:
+                if req.context.response_url is not None and callable(req.context.respond):


I don't think this should change the current behavior of the code base and it should make it more resilient, but asserting callable is another option

Suggested change

if req.context.response_url is not None and callable(req.context.respond):

if req.context.response_url is not None:

assert callable(req.context.respond), "req.context.respond must be callable"

I am not a big fan of assert statements in production code because the error often does not provide sufficient information to library users. Also, I think it's fine to assume context.respond is propertly set up if response_url exists.

Sounds good 💯 I will remove the assert changes and add type: ignore statements when needed

WilliamBergamin · 2024-08-16T17:12:36Z

slack_bolt/oauth/async_oauth_flow.py

@@ -69,15 +57,16 @@ def __init__(
            raise BoltError(error_oauth_settings_invalid_type_async())
        self.settings = settings

-        self.settings.logger = self._logger
+        if self._logger is not None:
+            self.settings.logger = self._logger


self.settings.logger should never be None

filmaj · 2024-08-16T18:59:30Z

My goodness from 11 minutes to 40 seconds 🤯

filmaj

Nicely done! The speed boosts are significant so it seems like a good change, but I will defer to Kaz's opinion on this - so will not put a checkmark on it 😄 sorry!

filmaj · 2024-08-16T20:04:20Z

slack_bolt/app/app.py

@@ -108,7 +109,7 @@ def __init__(
        user_facing_authorize_error_message: Optional[str] = None,
        installation_store: Optional[InstallationStore] = None,
        # for either only bot scope usage or v1.0.x compatibility
-        installation_store_bot_only: Optional[bool] = None,
+        installation_store_bot_only: bool = False,


Seems odd to have an optional boolean, unless the None state has a special meaning and so installation_store_bot_only is meant to have one of three states?

seratch

@WilliamBergamin Great work!

The reasons why I chose pytype over mypy during the initial development of bolt-python a few years ago are:

I compared mypy, pyright, and pytype; for this project, pytype was the most intelligent. It detected many type hint issues by analyzing method calls within bolt-python and test code. Slow execution was already a concern, but running it on CI builds was acceptable. Pyright looked good too, but it was not as helpful for this project in terms of the number of error/mistake detections.
Back then, mypy displayed way more unhelpful errors than other tools. During active development, I didn't have time to handle that. Also, pytype was significantly smarter for many patterns then.

Pytype was incredibly helpful, especially for the initial development, but I notice that it does not detect many issues anymore because this project is very stable and there are fewer type hint errors now. For this stage, I am open to prioritizing execution speed over the linter's intelligence.

seratch · 2024-08-19T07:26:05Z

slack_bolt/app/app.py

@@ -108,7 +109,7 @@ def __init__(
        user_facing_authorize_error_message: Optional[str] = None,
        installation_store: Optional[InstallationStore] = None,
        # for either only bot scope usage or v1.0.x compatibility
-        installation_store_bot_only: Optional[bool] = None,
+        installation_store_bot_only: bool = False,


@WilliamBergamin
This is intentional. When it's None, it means "unset" for the top-level parameter. L310 checks if it's explicitly set or not, and if it's set, then it compares the set value with "oauth_flow.settings.installation_store_bot_only." Developers could set both, thus implementing it this way to prioritize the top-level "installation_store_bot_only" if both are intentionally set. Don't change this logic. Adding some comments should be helpful for future maintainers!

seratch · 2024-08-19T07:28:18Z

slack_bolt/app/app.py

@@ -191,7 +192,8 @@ def message_hello(message, say):
            listener_executor: Custom executor to run background tasks. If absent, the default `ThreadPoolExecutor` will
                be used.
        """
-        signing_secret = signing_secret or os.environ.get("SLACK_SIGNING_SECRET", "")
+        if signing_secret is None:


It sounds os.environ.get(name, default_value)'s typing is not smart enough but this makes sense

seratch · 2024-08-19T07:36:54Z

slack_bolt/app/app.py

@@ -228,7 +230,7 @@ def message_hello(message, say):

        self._before_authorize: Optional[Middleware] = None
        if before_authorize is not None:
-            if isinstance(before_authorize, Callable):
+            if callable(before_authorize):


I see. mypy does not like this. It's okay to change this pattern

slack_bolt/app/app.py

seratch · 2024-08-19T07:41:00Z

slack_bolt/app/app.py

@@ -413,7 +417,7 @@ def _init_middleware_list(
                )
            else:
                raise BoltError(error_token_required())
-        else:
+        elif self._authorize is not None:


Good catch (by mypy); if we change this branching to elif, having else clause, which raises an exception, would be a plus

seratch · 2024-08-19T07:44:42Z

slack_bolt/listener/async_listener_error_handler.py

@@ -48,6 +48,7 @@ async def handle(
        )
        returned_response = await self.func(**kwargs)
        if returned_response is not None and isinstance(returned_response, BoltResponse):
+            assert response is not None, "response should never be 'None' here"


Is this intentional? The change seems unrelated to mypy migration. let's do this in a different PR

This change was intentional, mypy believes that response can be None in this situation
I've reverted the assert statements in favor of type: ignore

slack_bolt/logger/messages.py

seratch · 2024-08-19T07:51:17Z

slack_bolt/middleware/authorization/async_multi_teams_authorization.py

@@ -96,7 +96,7 @@ async def async_process(
                    "Although the app should be installed into this workspace, "
                    "the AuthorizeResult (returned value from authorize) for it was not found."
                )
-                if req.context.response_url is not None:
+                if req.context.response_url is not None and callable(req.context.respond):


I am not a big fan of assert statements in production code because the error often does not provide sufficient information to library users. Also, I think it's fine to assume context.respond is propertly set up if response_url exists.

slack_bolt/request/internals.py

WilliamBergamin · 2024-08-20T15:29:04Z

@seratch thank you for sharing the context behind originally choosing pytype over mypy, now that I've used both libraries I see how pytype is more intelligent and covers more patterns then mypy, I assume this gap was larger in the past. I see how useful this can be in the early stages this project 🙇

Simultaneously mypy did catch a few edge cases that I was not aware of and may be worth addressing in the future. For example the context.client is optional this has ripple effects in the project since it regularly assumes the client value is not None

seratch

Left one last comment about the error message string. After resolving it, you can merge this PR tomorrow in your timezone! Thanks for making these changes!

seratch · 2024-08-20T22:56:54Z

slack_bolt/app/app.py

@@ -413,7 +417,7 @@ def _init_middleware_list(
                )
            else:
                raise BoltError(error_token_required())
-        else:
+        elif self._authorize is not None:


The error message looks good to me. For consistency, move the string to slack_bolt.logger.messages like other parts in this class do!

seratch · 2024-08-20T22:57:16Z

slack_bolt/app/app.py

@@ -422,6 +426,9 @@ def _init_middleware_list(
                    user_facing_authorize_error_message=user_facing_authorize_error_message,
                )
            )
+        else:
+            raise BoltError("OAuthFlow or Authorize must be configured to make a Bolt app")


move the string to slack_bolt.logger.messages

seratch · 2024-08-20T22:57:37Z

slack_bolt/app/async_app.py

@@ -421,6 +425,8 @@ def _init_async_middleware_list(
                    user_facing_authorize_error_message=user_facing_authorize_error_message,
                )
            )
+        else:
+            raise BoltError("OAuthFlow or Authorize must be configured to make a Bolt app")


same as above

seratch · 2024-08-21T07:27:46Z

@WilliamBergamin Can you merge #1132 first if it looks good to you? The changes by that PR may cause conflicts with this PR. If that's the case, please resolve it on this PR side 🙏

feat: use mypy instead of pytype

1c441f2

WilliamBergamin added enhancement New feature or request area:async area:adapter labels Aug 15, 2024

WilliamBergamin self-assigned this Aug 15, 2024

WilliamBergamin added 3 commits August 16, 2024 11:22

Minimal improvements

c5e367f

Improve error surfacing

0af5174

Improve configuration and imports ignores

aa21432

WilliamBergamin commented Aug 16, 2024

View reviewed changes

WilliamBergamin added this to the 1.21.0 milestone Aug 16, 2024

WilliamBergamin changed the title ~~chore: move to mypy~~ health: move to mypy Aug 16, 2024

WilliamBergamin marked this pull request as ready for review August 16, 2024 18:15

WilliamBergamin requested review from seratch and filmaj August 16, 2024 18:15

WilliamBergamin mentioned this pull request Aug 16, 2024

health: move to mypy slackapi/python-slack-hooks#50

Merged

3 tasks

filmaj reviewed Aug 16, 2024

View reviewed changes

seratch requested changes Aug 19, 2024

View reviewed changes

WilliamBergamin added 4 commits August 19, 2024 12:11

revert optional installation_store_bot_only

2dac98b

Add proper exception raising

1fe33a1

Revert assert statements

e31fa56

Improve based on feedback

b1a17a1

WilliamBergamin requested a review from seratch August 19, 2024 18:18

seratch approved these changes Aug 20, 2024

View reviewed changes

WilliamBergamin and others added 2 commits August 21, 2024 10:05

Improve error surfacing

4507ee2

Merge branch 'main' into move-to-my-py

c002770

WilliamBergamin merged commit 9dcc0fb into slackapi:main Aug 21, 2024
12 checks passed

WilliamBergamin deleted the move-to-my-py branch August 21, 2024 14:40

WilliamBergamin mentioned this pull request Dec 20, 2024

health: move to mypy slackapi/python-slack-sdk#1620

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

health: move to mypy #1130

health: move to mypy #1130

WilliamBergamin commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading

WilliamBergamin Aug 16, 2024

filmaj Aug 16, 2024

seratch Aug 19, 2024

WilliamBergamin Aug 19, 2024

WilliamBergamin Aug 16, 2024

seratch Aug 19, 2024

WilliamBergamin Aug 16, 2024

seratch Aug 19, 2024

WilliamBergamin Aug 19, 2024

seratch Aug 20, 2024

WilliamBergamin Aug 16, 2024

seratch Aug 19, 2024

WilliamBergamin Aug 19, 2024

WilliamBergamin Aug 16, 2024

filmaj commented Aug 16, 2024

filmaj left a comment

filmaj Aug 16, 2024

seratch left a comment

seratch Aug 19, 2024

seratch Aug 19, 2024

seratch Aug 19, 2024

seratch Aug 19, 2024

seratch Aug 19, 2024

WilliamBergamin Aug 19, 2024 •

edited

Loading

seratch Aug 19, 2024

WilliamBergamin commented Aug 20, 2024

seratch left a comment

seratch Aug 20, 2024

seratch Aug 20, 2024

seratch Aug 20, 2024

seratch commented Aug 21, 2024

	if req.context.response_url is not None and callable(req.context.respond):
	if req.context.response_url is not None:
	assert callable(req.context.respond), "req.context.respond must be callable"

health: move to mypy #1130

health: move to mypy #1130

Conversation

WilliamBergamin commented Aug 15, 2024 • edited Loading

Category

Requirements

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj commented Aug 16, 2024

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin commented Aug 20, 2024

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Aug 21, 2024

WilliamBergamin commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading

WilliamBergamin Aug 19, 2024 •

edited

Loading