Skip to content

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented Feb 23, 2021

This pull request improves the consistency of logging in Bolt. Thanks to @eddyg's feedback in the community workspace: https://community.slack.com/archives/CHL4CLRCZ/p1614000712009600 (if you're not a member of the workspace, you can join from here)

eddyg Feb 22nd at 10:31 PM
Anybody know why create_async_web_client doesn’t take an (optional) logger argument and pass it to AsyncWebClient()? I know AsyncBaseClient has an optional logger arg. so I know I could init my own and pass it as a client arg to AsyncApp(), but I wondered if there was a reason Bolt doesn’t pass down a logger by default? (edited)

seratch:slack: 35 minutes ago
@eddyg good point. actually the logger argument in WebClient was added recently (1 month ago). there is not reason not to update the bolt code.

Screen Shot 2021-02-24 at 08 05 35

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.

@seratch seratch force-pushed the fix-client-default-logger branch from 2f47812 to 7374d29 Compare February 23, 2021 23:51
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #246 (7374d29) into main (3e483fe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #246   +/-   ##
=======================================
  Coverage   91.36%   91.37%           
=======================================
  Files         160      160           
  Lines        4971     4973    +2     
=======================================
+ Hits         4542     4544    +2     
  Misses        429      429           
Impacted Files Coverage Δ
...ck_bolt/adapter/aws_lambda/lambda_s3_oauth_flow.py 97.14% <100.00%> (ø)
slack_bolt/app/app.py 86.88% <100.00%> (ø)
slack_bolt/app/async_app.py 94.73% <100.00%> (ø)
slack_bolt/oauth/async_oauth_flow.py 90.75% <100.00%> (ø)
slack_bolt/oauth/oauth_flow.py 90.43% <100.00%> (ø)
slack_bolt/util/async_utils.py 100.00% <100.00%> (ø)
slack_bolt/util/utils.py 94.11% <100.00%> (+0.17%) ⬆️

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 3e483fe...7374d29. Read the comment docs.

Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

Looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants