-
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1387 +/- ##
==========================================
- Coverage 85.57% 85.51% -0.07%
==========================================
Files 111 111
Lines 11586 11591 +5
==========================================
- Hits 9915 9912 -3
- Misses 1671 1679 +8
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
This looks good to me! Left a few comments for future maintenance, but nothing blocking – the checks for deprecated methods work wonderfully!
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 comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to raise the following error if SLACKCLIENT_SKIP_DEPRECATION
isn't set:
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch! will fix before merging it
|
||
def show_2020_01_deprecation(method_name: str): |
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.
Nice change!
@@ -10,8 +10,10 @@ | |||
"admin.conversations.whitelist.", | |||
] | |||
|
|||
deprecated_method_prefixes_2023_07 = ["stars."] |
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
see also: https://api.slack.com/changelog/2023-07-its-later-already-for-stars-and-reminders
Summary
(Describe the goal of this PR. Mention any related issue numbers)
Category (place an
x
in each of the[ ]
)/docs-src
(Documents, have you run./scripts/docs.sh
?)/docs-src-v2
(Documents, have you run./scripts/docs-v2.sh
?)/tutorial
(PythOnBoardingBot tutorial)tests
/integration_tests
(Automated tests for this library)Requirements (place an
x
in each[ ]
)python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.