-
Notifications
You must be signed in to change notification settings - Fork 834
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
Mark stars.* API methods as deprecated #1387
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,10 @@ | |
"admin.conversations.whitelist.", | ||
] | ||
|
||
deprecated_method_prefixes_2023_07 = ["stars."] | ||
|
||
def show_2020_01_deprecation(method_name: str): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice change! |
||
|
||
def show_deprecation_warning_if_any(method_name: str): | ||
"""Prints a warning if the given method is deprecated""" | ||
|
||
skip_deprecation = os.environ.get("SLACKCLIENT_SKIP_DEPRECATION") # for unit tests etc. | ||
|
@@ -20,6 +22,7 @@ def show_2020_01_deprecation(method_name: str): | |
if not method_name: | ||
return | ||
|
||
# 2020/01 conversations API deprecation | ||
matched_prefixes = [prefix for prefix in deprecated_method_prefixes_2020_01 if method_name.startswith(prefix)] | ||
if len(matched_prefixes) > 0: | ||
message = ( | ||
|
@@ -28,3 +31,12 @@ def show_2020_01_deprecation(method_name: str): | |
"https://api.slack.com/changelog/2020-01-deprecating-antecedents-to-the-conversations-api" | ||
) | ||
warnings.warn(message) | ||
|
||
# 2023/07 stars API deprecation | ||
matched_prefixes = [prefix for prefix in deprecated_method_prefixes_2023_07 if method_name.startswith(prefix)] | ||
if len(matched_prefixes) > 0: | ||
message = ( | ||
f"{method_name} is deprecated. For more info, go to " | ||
"https://api.slack.com/changelog/2023-07-its-later-already-for-stars-and-reminders" | ||
) | ||
warnings.warn(message) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import os | ||
import unittest | ||
import pytest | ||
|
||
from slack_sdk.web import WebClient | ||
from tests.slack_sdk.web.mock_web_api_server import ( | ||
setup_mock_web_api_server, | ||
cleanup_mock_web_api_server, | ||
) | ||
|
||
|
||
class TestWebClient(unittest.TestCase): | ||
def setUp(self): | ||
setup_mock_web_api_server(self) | ||
|
||
def tearDown(self): | ||
cleanup_mock_web_api_server(self) | ||
|
||
# You can enable this test to verify if the warning can be printed as expected | ||
@pytest.mark.skip() | ||
def test_stars_deprecation(self): | ||
env_value = os.environ.get("SLACKCLIENT_SKIP_DEPRECATION") | ||
try: | ||
os.environ.pop("SLACKCLIENT_SKIP_DEPRECATION") | ||
client = WebClient(base_url="http://localhost:8888") | ||
client.stars_list(token="xoxb-api_test") | ||
finally: | ||
os.environ.update({"SLACKCLIENT_SKIP_DEPRECATION": env_value}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to raise the following error if TypeError: str expected, not NoneType Not a blocker IMO since the deprecation warning is correctly shown when setting this variable before running the test, just wanted to raise this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, good catch! will fix before merging it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we want to include a comment to the changelog above this deprecation too? It's already included in the comment below so not a huge deal if not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, we don't have any workaround for this breaking change. So, I cannot think of any good comment for it