-
Notifications
You must be signed in to change notification settings - Fork 850
chore: remove 3.6 references #1735
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 84.91% 84.88% -0.04%
==========================================
Files 113 113
Lines 12895 12895
==========================================
- Hits 10950 10946 -4
- Misses 1945 1949 +4 ☔ View full report in Codecov by Sentry. |
WilliamBergamin
left a comment
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.
Great work 💯 thanks for putting this together
I left a few comments they are mainly minor ones 🚀 these dependency support version type changes are always more complicated then expected
README.md
Outdated
| --- | ||
|
|
||
| This library requires Python 3.6 and above. If you require Python 2, please use our [SlackClient - v1.x][slackclientv1]. If you're unsure how to check what version of Python you're on, you can check it using the following: | ||
| This library requires Python 3.7 and above. If you require Python 2, please use our [SlackClient - v1.x][slackclientv1]. If you're unsure how to check what version of Python you're on, you can check it using the following: |
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.
We should also remove the python 2 sentence 😅 the slackClient - v1.x is deprecated
| This library requires Python 3.7 and above. If you require Python 2, please use our [SlackClient - v1.x][slackclientv1]. If you're unsure how to check what version of Python you're on, you can check it using the following: | |
| This library requires Python 3.7 and above. If you're unsure how to check what version of Python you're on, you can check it using the following: |
docs/english/legacy/index.md
Outdated
| Slack APIs allow anyone to build full featured integrations that extend and expand the capabilities of your Slack workspace. These APIs allow you to build applications that interact with Slack just like the people on your team. They can post messages, respond to events that happen, and build complex UIs for getting work done. | ||
|
|
||
| To make it easier for Python programmers to build Slack applications, we've provided this open source SDK that will help you get started building Python apps as quickly as possible. The current version is built for Python 3.6 and higher — if you need to target Python 2.x, you might consider using v1 of the SDK. | ||
| To make it easier for Python programmers to build Slack applications, we've provided this open source SDK that will help you get started building Python apps as quickly as possible. The current version is built for Python 3.7 and higher — if you need to target Python 2.x, you might consider using v1 of the SDK. |
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.
Same here
| To make it easier for Python programmers to build Slack applications, we've provided this open source SDK that will help you get started building Python apps as quickly as possible. The current version is built for Python 3.7 and higher — if you need to target Python 2.x, you might consider using v1 of the SDK. | |
| To make it easier for Python programmers to build Slack applications, we've provided this open source SDK that will help you get started building Python apps as quickly as possible. The current version is built for Python 3.7 and higher. |
docs/english/v3-migration.md
Outdated
| ### Minimum Python versions {#minimum-versions} | ||
|
|
||
| `slackclient` v2.x requires Python 3.6 (or higher). Support for Python 2.7 is maintained in the existing `slackclient` v1.x. | ||
| `slackclient` v2.x requires Python 3.7 (or higher). Support for Python 2.7 is maintained in the existing `slackclient` v1.x. |
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.
Same here
| `slackclient` v2.x requires Python 3.7 (or higher). Support for Python 2.7 is maintained in the existing `slackclient` v1.x. | |
| `slackclient` v2.x requires Python 3.7 (or higher). |
| self.assertEqual(0, after - before) | ||
|
|
||
| # fails with Python 3.6 | ||
| # fails with Python 3.6 (no longer supported) |
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.
I think we can remove this comment entirely 💯
| self.assertEqual(0, after - before) | ||
|
|
||
| # fails with Python 3.6 | ||
| # fails with Python 3.6 (no longer supported) |
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 one also 🚀
requirements/testing.txt
Outdated
| black>=22.8.0; python_version=="3.7" | ||
| black==22.10.0; python_version>="3.8" |
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.
I think this might be the reason why the CI tests are failing 🤔
Since we aren't supporting 3.6 anymore we can remove the special case and just keep a global one, like black==22.10.0, but let me know what you think?
| black>=22.8.0; python_version=="3.7" | |
| black==22.10.0; python_version>="3.8" | |
| black==22.10.0; |
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.
Ah yes I agree. Black v22.10.0 supports python 3.7 so the split requirement doesn’t seem to be needed anymore!
slack_sdk/web/internal_utils.py
Outdated
| Returns: | ||
| The user agent string. | ||
| e.g. 'Python/3.6.7 slackclient/2.0.0 Darwin/17.7.0' | ||
| e.g. 'Python/3.7.0 slackclient/2.0.0 Darwin/17.7.0' |
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.
NIT: lets use 3.7.17 since its the latest 3.7 version and a security release
slack_sdk/web/internal_utils.py
Outdated
| 'Content-Type': 'application/json;charset=utf-8', | ||
| 'Authorization': 'Bearer xoxb-1234-1243', | ||
| 'User-Agent': 'Python/3.6.8 slack/2.1.0 Darwin/17.7.0' | ||
| 'User-Agent': 'Python/3.7.0 slack/2.1.0 Darwin/17.7.0' |
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.
NIT: lets use 3.7.17 since its the latest 3.7 version and a security release
| validate_aiohttp_version("2.1.3", print) | ||
| self.assertEqual(state["counter"], 1) | ||
| validate_aiohttp_version("3.6.3", print) | ||
| validate_aiohttp_version("3.7.0", print) |
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.
I think we want to keep this since this test covers "not recommended versions"
tutorial/README.md
Outdated
| ``` | ||
| $ python3 --version | ||
| -> Python 3.6.7 | ||
| -> Python 3.7.0 |
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.
NIT: lets use 3.7.17 since its the latest 3.7 version and a security release
WilliamBergamin
left a comment
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 for addressing all the comments 💯 🚀
There are 2 minor ones left that related to code comments, once addressed, we should be all clear and ready to merge 🚢
slack/web/async_internal_utils.py
Outdated
| 'Content-Type': 'application/json;charset=utf-8', | ||
| 'Authorization': 'Bearer xoxb-1234-1243', | ||
| 'User-Agent': 'Python/3.6.8 slack/2.1.0 Darwin/17.7.0' | ||
| 'User-Agent': 'Python/3.7.0 slack/2.1.0 Darwin/17.7.0' |
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.
NIT: what do you think about keeping this consistent with slack_sdk/web/internal_utils.py
| 'User-Agent': 'Python/3.7.0 slack/2.1.0 Darwin/17.7.0' | |
| 'User-Agent': 'Python/3.7.17 slack/2.1.0 Darwin/17.7.0' |
WilliamBergamin
left a comment
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 work 💯
|
Once slackapi/bolt-python#1359 is merged and release we can merge and release these changes 🚀 |
Summary
Remove Python 3.6 support by updating all references to reflect the minimum of Python 3.7+.
Testing
n/a
Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.