Fix #754 by adding the async version of Tornado adapter #758
Conversation
| @@ -9,9 +9,9 @@ | |||
|
|
|||
|
|
|||
| @app.middleware # or app.use(log_request) | |||
| def log_request(logger, body, next): | |||
| def log_request(logger, body, next_): | |||
There was a problem hiding this comment.
just a minor improvement to eliminate warnings by IDE. See also: #394
Codecov Report
@@ Coverage Diff @@
## main #758 +/- ##
==========================================
- Coverage 92.06% 92.02% -0.05%
==========================================
Files 172 173 +1
Lines 5875 5906 +31
==========================================
+ Hits 5409 5435 +26
- Misses 466 471 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Great work on this, the code looks good to me 🥇
I ran some test locally that modify the behavior in order to deploy a sync App on the asyncTornado adapter and this seemed to work 😮 I believe the async handler just executes the sync code as a block similarly to how it would await for the async_dispatch
I'm not sure if this is behavior we want in this adapter, but I think it could be possible for this adapter to accept async App and sync Apps
|
@WilliamBergamin Sorry, I don't understand your point yet. Can you elaborate more on the changes you are requesting in your comment? The intention to add a new adapter in this PR is to enable developers to use AsyncApp along with Tornado. Since AsyncApp relies on AIOHTTP, we need to have a separate module for it. Also, AsyncApp runs "async" functions for handling requests from Slack. This is completely different from the App approach. Let me know if I am missing something here. |
I clearly didn't answer this but indeed it's technically possible to accept both App and AsyncApp in this new adapter. However, perhaps no one will use it for App user cases. Also, even when a single handler supports both, two separate handlers should exist for sync App users who do not need AIOHTTP at all. Again, just importing AsyncApp requires AIOHTTP installation, which is optional dependency of slack-bolt/slack-sdk. In addition, for consistency with other built-in adapters, I prefer supporting only AsyncApp in this case. Let's discuss more on this next week. |
|
@seratch this makes a lot of sense thank you for the answer 💯 |
This pull request resolves #754 by adding the async version of Tornado adapter. Refer to #754 for the context and details.
Category (place an
xin each of the[ ])slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements (place an
xin each[ ])Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.