Skip to content

Conversation

@zimeg
Copy link
Member

@zimeg zimeg commented Sep 30, 2025

Summary

This PR uses the JSON content type to send structured values, such as an array of loading_messages to the server with correct encodings 🤖

Testing

Test cases now include arguments that require this json content type!

Category

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • tests/integration_tests (Automated tests for this library)

Requirements

  • 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 python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@zimeg zimeg requested a review from mwbrooks September 30, 2025 17:02
@zimeg zimeg self-assigned this Sep 30, 2025
@zimeg zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented tests M-T: Testing work only semver:patch web-client area:async labels Sep 30, 2025
@codecov
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (ai-apps@f901e25). Learn more about missing BASE report.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             ai-apps    #1754   +/-   ##
==========================================
  Coverage           ?   85.02%           
==========================================
  Files              ?      113           
  Lines              ?    12958           
  Branches           ?        0           
==========================================
  Hits               ?    11017           
  Misses             ?     1941           
  Partials           ?        0           

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

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.

✅ Thanks for catching this @mcodik and fixing it @zimeg!

🧠 We should do a quick pass to make sure that we're not using params anywhere else. Then we may want to bring out another Python SDK Beta release for folks.

@zimeg
Copy link
Member Author

zimeg commented Sep 30, 2025

We should do a quick pass to make sure that we're not using params anywhere else. Then we may want to bring out another Python SDK Beta release for folks.

@mwbrooks This is a good call. I'll take a pass at this before merging:

  • assistant.threads.setStatus: 👁️‍🗨️ Updated in this PR!

return self.api_call("assistant.threads.setStatus", params=kwargs)

  • chat.appendStream:

return self.api_call("chat.appendStream", json=kwargs)

  • chat.startStream:

return self.api_call("chat.startStream", json=kwargs)

  • chat.stopStream:

return self.api_call("chat.stopStream", json=kwargs)

📚 AFAICT other methods can require either params or json depending on the attributes accepted and the actual API handlers. I believe "json" is required when structured values - not "flat" top-level strings - are used here!

@zimeg
Copy link
Member Author

zimeg commented Sep 30, 2025

@mwbrooks @mcodik Thanks both so much for the kind findings and fast review! Let's merge this now 🚢 💨

@zimeg zimeg merged commit 829d409 into ai-apps Sep 30, 2025
12 checks passed
@zimeg zimeg deleted the zimeg-fix-web-json-loading-states branch September 30, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:async bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch tests M-T: Testing work only web-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants