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.

Sign up for GitHub

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

Jump to bottom

Fix #1302 by updating the warning logs in WebClient to be consistent with Node SDK #1303

Merged
merged 2 commits into from
Nov 18, 2022
Merged

Fix #1302 by updating the warning logs in WebClient to be consistent with Node SDK #1303

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0c2bbb
The implementation of #1208 is different from the node-slack-sdk vers…
mar3mar3 Nov 18, 2022
a063b08
Update slack_sdk/web/internal_utils.py
mar3mar3 Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 20 additions & 19 deletions slack_sdk/web/internal_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,38 +255,39 @@ def _warn_if_text_or_attachment_fallback_is_missing(endpoint: str, kwargs: Dict[
if skip_deprecation:
return

# At this point, at a minimum, text argument is missing. Warn the user about this.
message = (
# if text argument is missing, Warn the user about this.
# However, do not warn if a fallback field exists for all attachments, since this can be substituted.
missing_text_message = (
f"The top-level `text` argument is missing in the request payload for a {endpoint} call - "
f"It's a best practice to always provide a `text` argument when posting a message. "
f"The `text` argument is used in places where content cannot be rendered such as: "
"system push notifications, assistive technology such as screen readers, etc."
)
warnings.warn(message, UserWarning)

# https://api.slack.com/reference/messaging/attachments
# Check if the fallback field exists for all the attachments
# Not all attachments have a fallback property; warn about this too!
missing_fallback_message = (
f"Additionally, the attachment-level `fallback` argument is missing in the request payload for a {endpoint} call"
" - To avoid this warning, it is recommended to always provide a top-level `text` argument when posting a"
" message. Alternatively you can provide an attachment-level `fallback` argument, though this is now considered"
" a legacy field (see https://api.slack.com/reference/messaging/attachments#legacy_fields for more details)."
)

# Additionally, specifically for attachments, there is a legacy field available at the attachment level called `fallback`
# Even with a missing text, one can provide a `fallback` per attachment.
# More details here: https://api.slack.com/reference/messaging/attachments#legacy_fields
attachments = kwargs.get("attachments")
# Note that this method does not verify attachments
# if the value is already serialized as a single str value.
if (
attachments is not None
and isinstance(attachments, list)
and not all(
if attachments is not None and isinstance(attachments, list):
Copy link
Member

Choose a reason for hiding this comment

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

Have you applied black code formatter to this change? If not, can you run pip install black && black slack_sdk/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that.
By the way, I'm editing in IntelliJ.

$pip install black && black slack_sdk/
Requirement already satisfied: black in ./venv/lib/python3.11/site-packages (22.8.0)
Requirement already satisfied: click>=8.0.0 in ./venv/lib/python3.11/site-packages (from black) (8.0.4)
Requirement already satisfied: platformdirs>=2 in ./venv/lib/python3.11/site-packages (from black) (2.5.4)
Requirement already satisfied: pathspec>=0.9.0 in ./venv/lib/python3.11/site-packages (from black) (0.10.2)
Requirement already satisfied: mypy-extensions>=0.4.3 in ./venv/lib/python3.11/site-packages (from black) (0.4.3)
All done! ✨ 🍰 ✨
113 files left unchanged.

Copy link
Member

@seratch seratch Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!

By the way, I'm editing in IntelliJ

I like this. I am a long-time user of PyCharm for Python too!

if not all(
[isinstance(attachment, dict) and len(attachment.get("fallback", "").strip()) > 0 for attachment in attachments]
)
):
# https://api.slack.com/reference/messaging/attachments
# Check if the fallback field exists for all the attachments
# Not all attachments have a fallback property; warn about this too!
message = (
f"Additionally, the attachment-level `fallback` argument is missing in the request payload for a {endpoint} call"
f" - To avoid this warning, it is recommended to always provide a top-level `text` argument when posting a"
f" message. Alternatively you can provide an attachment-level `fallback` argument, though this is now considered"
f" a legacy field (see https://api.slack.com/reference/messaging/attachments#legacy_fields for more details)."
)
warnings.warn(message, UserWarning)
):
warnings.warn(missing_text_message, UserWarning)
warnings.warn(missing_fallback_message, UserWarning)
else:
warnings.warn(missing_text_message, UserWarning)


def _build_unexpected_body_error_message(body: str) -> str:
Expand Down