Skip to content

fix(falcon/async_resource): replace the use of removed resp.body - #1516

Merged
WilliamBergamin merged 2 commits into
slackapi:mainfrom
vytas7:asyncresource-set-404-resp.text
Jun 1, 2026
Merged

fix(falcon/async_resource): replace the use of removed resp.body#1516
WilliamBergamin merged 2 commits into
slackapi:mainfrom
vytas7:asyncresource-set-404-resp.text

Conversation

@vytas7

Copy link
Copy Markdown
Contributor

Summary

Testing

Category

  • 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

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.

@vytas7
vytas7 requested a review from a team as a code ownerMay 30, 2026 17:49
@salesforce-cla

Copy link
Copy Markdown

Thanks for the contribution! Before we can merge this, we need @vytas7 to sign the Salesforce Inc. Contributor License Agreement.

return

resp.status = "404"
# Falcon 4.x w/ mypy fails to correctly infer the str type here

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Mypy was actually functioning correctly - resp.body was removed long ago.

(All this could be replaced with a onliner raising HTTPNotFound, especially if you're OK with a JSON error.)

@vytas7

Copy link
Copy Markdown
ContributorAuthor

(I believe I have now signed the CLA.)

@codecov

codecovBot commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.36%. Comparing base (e8222b5) to head (c90d5f9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@ Coverage Diff @@## main #1516 +/- ##
==========================================
+ Coverage 91.30% 91.36% +0.05% 
==========================================
Files 228 228 Lines 7271 7275 +4 ==========================================
+ Hits 6639 6647 +8 + Misses 632 628 -4 

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

@WilliamBergamin

WilliamBergamin commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for this contribution and interest in our project 🙏

Sometimes we need to close and reopen the issue to force the CLA tool to retrigger

@WilliamBergaminWilliamBergamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your time an interest in our project 🙏

I mirrored the changes in the sync implementation and added unit tests to cover this logic

@WilliamBergamin
WilliamBergamin enabled auto-merge (squash) June 1, 2026 13:35
@WilliamBergamin
WilliamBergamin merged commit a38510f into slackapi:mainJun 1, 2026
16 checks passed
@vytas7

vytas7 commented Jun 4, 2026

Copy link
Copy Markdown
ContributorAuthor

@WilliamBergamin thanks!

Note that resp.text won't work with Falcon 2.0, so maybe we could just tighten the requirements? (Falcon 2.0 is 7 years old now.)

Alternatively one can set resp.data to a bytestring; that already exists even in the 1.x.

@zimegzimeg added bug Something isn't working area:adapter semver:patch labels Jun 4, 2026
@zimegzimeg added this to the 1.29.0 milestone Jun 4, 2026
Sign up for freeto 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.

3 participants

@vytas7@WilliamBergamin@zimeg