Skip to content
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.

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

Allow a custom Executor in App #453

Merged
merged 6 commits into from Aug 29, 2021

Conversation

chrisbouchard
Copy link
Contributor

@chrisbouchard chrisbouchard commented Aug 28, 2021

This pull request adds a listener_executor keyword parameter to App.__init__, so that the executor for running background listener functions can be customized by the user. It also weakens the type requirement for executors to just concurrent.futures.Executor, the abstract base class of executors.

Fixes #452.

Category (place an x in each of the [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in 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.

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

It's not *required* that the executor be a ThreadPoolExecutor -- any
thread-based executor meeting the Executor interface would work. We can
weaken the type annotation to allow custom executors.
@CLAassistant
Copy link

CLAassistant commented Aug 28, 2021

CLA assistant check
All committers have signed the CLA.

@chrisbouchard chrisbouchard marked this pull request as ready for review August 28, 2021 17:25
@chrisbouchard
Copy link
Contributor Author

I think this is ready for review. One thing I'm unsure of is whether I should update the API documentation. I did update the docstring on App.__init__. When I ran ./scripts/generate_api_docs.sh it updated way more files than I touched.

@mwbrooks mwbrooks requested a review from seratch August 28, 2021 21:05
@mwbrooks mwbrooks added this to the 1.9.0 milestone Aug 28, 2021
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

Hey @chrisbouchard, thanks for following up with a pull request! You're awesome 🎉

The CLA looks good and I've enabled the test workflow to run. ✅

I'll let @seratch review your pull request and comment on why generating the documentation changes more files than expected. Regardless, thanks a lot for taking the extra steps to add test and update the docs 🤘🏻

@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #453 (fea77b9) into main (1272f89) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #453   +/-   ##
=======================================
  Coverage   91.34%   91.35%           
=======================================
  Files         167      167           
  Lines        5501     5503    +2     
=======================================
+ Hits         5025     5027    +2     
  Misses        476      476           
Impacted Files Coverage Δ
slack_bolt/app/app.py 87.64% <100.00%> (+0.05%) ⬆️
slack_bolt/lazy_listener/thread_runner.py 100.00% <100.00%> (ø)
slack_bolt/listener/thread_runner.py 93.69% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1272f89...fea77b9. Read the comment docs.

@chrisbouchard
Copy link
Contributor Author

chrisbouchard commented Aug 29, 2021

I was able to figure out the issue with docs. It looks like a new version of pydoc has been published (0.10.0) that causes content and formatting changes in around 171 doc files. I fixed my pydoc version to 0.9.2 to match what's currently checked into main and I got a much more reasonable diff.

@seratch
Copy link
Member

seratch commented Aug 29, 2021

@chrisbouchard Thanks for updating the docs but can you revert the change? We usually update the API docs after releasing a version.

slack_bolt/app/app.py Outdated Show resolved Hide resolved
slack_bolt/app/app.py Show resolved Hide resolved
Since it will be used by pydoc to generate the docs, we should use
markdown for formatting.
Improved wording for documentation around the new `listener_executor` parameter.

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@chrisbouchard
Copy link
Contributor Author

Doc changes reverted, and wording suggestions applied. 👍

@seratch
Copy link
Member

seratch commented Aug 29, 2021

Thanks for working on this! LGTM

@seratch seratch merged commit 6684df6 into slackapi:main Aug 29, 2021
@chrisbouchard chrisbouchard deleted the feature/app_executor branch August 30, 2021 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow App's listener_executor to be supplied to __init__
4 participants