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

Fix #1204 Deprecation Warnings in Slack Bolt Integration with Sanic #1220

Closed
wants to merge 3 commits into from

Conversation

HTSagara
Copy link
Contributor

@HTSagara HTSagara commented Dec 6, 2024

This Pull Request addresses #1204 deprecation warnings and type-related errors that occur when integrating Slack Bolt with Sanic. The warnings were caused by the use of Sanic's outdated dict-based cookie handling syntax, which has been deprecated since Sanic v23.3. These issues surfaced during testing and when handling requests.
Key Changes:

**Updated Cookie Handling: **

  • Replaced the old dict-style cookie setting syntax with Sanic's recommended add_cookie method.
  • Ensured proper type handling for cookie attributes such as path, domain, expires, and max-age.

Resolved Type Errors:

  • Explicitly converted path and domain attributes to strings or assigned appropriate defaults.
  • Handled expires and max-age fields gracefully to align with Sanic's requirements.

Testing:

  • The changes have been validated through existing test cases

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.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.96%. Comparing base (4eb3855) to head (d47a5e7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
+ Coverage   90.94%   90.96%   +0.02%     
==========================================
  Files         222      222              
  Lines        7506     7501       -5     
==========================================
- Hits         6826     6823       -3     
+ Misses        680      678       -2     

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

@hello-ashleyintech
Copy link

@HTSagara Thanks for submitting this PR! 🙌 I see that your base branch is out of date with main, would you be able to merge the latest main into this branch and push it up?

@HTSagara
Copy link
Contributor Author

HTSagara commented Dec 9, 2024

@hello-ashleyintech thank you for the feedback. I just updated my branch with the main

@seratch seratch changed the title Fix Deprecation Warnings in Slack Bolt Integration with Sanic - Issue#1204 Fix #1204 Deprecation Warnings in Slack Bolt Integration with Sanic Dec 11, 2024
@seratch seratch added this to the 1.21.4 milestone Dec 11, 2024
@seratch seratch self-assigned this Dec 11, 2024
@seratch
Copy link
Member

seratch commented Dec 11, 2024

@HTSagara Thank you so much for taking the time to make these changes! I've added a few additional changes (= removed the comments repeating what the code does, added assertions in the Sanic unit tests to verify the cookie data behavior) and merged your changes as a single commit: 35a4b09

Thanks again for your contribution! 🎉

@seratch seratch closed this Dec 11, 2024
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.

3 participants