Skip to content

feat: support markdown_text parameter - #1718

Merged
WilliamBergamin merged 6 commits into
mainfrom
support-markdown-text
Aug 5, 2025
Merged

WilliamBergamin merged 6 commits into
mainfrom
support-markdown-text

Conversation

@WilliamBergamin

@WilliamBergamin WilliamBergamin commented Jul 31, 2025 •

Copy link
Copy Markdown
Contributor

Summary

#1713 reports that warnings are printed when the markdown_text parameter is passed without the text parameter, but passing text alongside markdown_text results in an API error markdown_text_conflict 😢 . This behavior affects chat_postMessage, chat_update, chat_scheduledMessage and chat_postEphemeral methods.

This PR aims to add support for the markdown_text parameter by omitting warnings when markdown_text is passed without text

Testing

Use the following app to test the functionality

import os
import logging

from slack_sdk import WebClient

logging.basicConfig(level=logging.INFO)

client = WebClient(token=os.environ.get("SLACK_BOT_TOKEN"))

markdown_content = (
    "*Welcome to Our Bolt Python Project!* :rocket:\n\n"
    ">This is a Slack app built with Bolt for Python.\n\n"
    "*Key features:*\n"
    "• Message handling\n"
    "• Event subscriptions\n"
    "• Interactive components\n"
    "• App Home customization\n\n"
    "```python\n"
    "# Quick setup\n"
    "python3 -m venv .venv\n"
    "source .venv/bin/activate\n"
    "pip install -r requirements.txt\n"
    "```\n\n"
    "Check out <https://api.slack.com/start/building/bolt-python|Bolt documentation> for more details."
)

response = client.chat_postMessage(
    channel="C111",
    text="hello",
    markdown_text=markdown_content
)

if response["ok"]:
    client.logger.info(f"Message posted successfully")
    client.logger.info(f"Channel: {response['channel']}")
    client.logger.info(f"Timestamp: {response['ts']}")
    client.logger.info(f"Message: {response['message']['text'][:50]}...")
else:
    client.logger.error(f"Error posting message: {response['error']}")
    client.logger.error(f"Full response: {response}")
  1. Including just the text argument should post a message as expected 🟢
  2. Including just the markdown_text argument should post a message as expected 🟢 no warnings
  3. Including the markdown_text and text arguments results in an API error markdown_text_conflict
  4. Excluding both arguments results in an API error no_text

Feedback

I've added the markdown_text argument in the methods definitions, but we could omit this since developers are able to pass this value through keyword arguments, should we explicitly define this argument?

Category

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • 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.

@WilliamBergamin WilliamBergamin added this to the 3.37.0 milestone Jul 31, 2025
@WilliamBergamin WilliamBergamin self-assigned this Jul 31, 2025
@WilliamBergamin WilliamBergamin added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality semver:minor web-client Version: 3x area:async labels Jul 31, 2025
@codecov

codecov Bot commented Jul 31, 2025 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.91%. Comparing base (8922dc8) to head (4fb424a).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
+ Coverage   84.90%   84.91%   +0.01%     
==========================================
  Files         113      113              
  Lines       12892    12895       +3     
==========================================
+ Hits        10946    10950       +4     
+ Misses       1946     1945       -1     

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

@ewanek1 ewanek1 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.

LGTM! 🙌 ☺

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@WilliamBergamin LGTM! And thanks for keeping outputs relevant with these markdown formats 🎁

I left a few comments below but like where this PR moves things as is 👾

link_names: Optional[bool] = None,
username: Optional[str] = None,
parse: Optional[str] = None,
markdown_text: Optional[str] = None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've added the markdown_text argument in the methods definitions, but we could omit this since developers are able to pass this value through keyword arguments, should we explicitly define this argument?

praise: Adding this I think is a nice improvement for the typeahead hints found in some editors with virtual environments!

thought: It's nice to know that missing arguments aren't blocking new features though 🤖 ✨

Comment thread slack_sdk/web/internal_utils.py Outdated
text = kwargs.get("text")
if text and len(text.strip()) > 0:
markdown_text = kwargs.get("markdown_text")
if (text and len(text.strip()) > 0) or (markdown_text and len(markdown_text.strip()) > 0):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion(non-blocking): Separating these cases as separate if statements and returns might make later additions more clear 👁️‍🗨️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yess I agree let me see how this looks 💯

Comment on lines +77 to +78
self.assertEqual(warning_list, [])
self.assertIsNone(resp["error"])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🪄 ✨

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question(non-blocking): Do we want to keep the issue_### pattern or can we change this to something like:

tests/web/test_web_client_warnings.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point let me rename this 💯

@WilliamBergamin
WilliamBergamin requested a review from zimeg August 1, 2025 21:03

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧪 ✨ Awesome changes more with the testing updates and additions! Please do feel free to merge when the time is right.

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 enhancement M-T: A feature request for new functionality semver:minor Version: 3x web-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants