-
Notifications
You must be signed in to change notification settings - Fork 844
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
Support pathlike objects in upload util #1656
base: main
Are you sure you want to change the base?
Conversation
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 like a nice enhancement 💯 thank you!
Any chance you could add unit tests for file passed as str
and os.PathLike
? if it is not trivial let me know I will take a look
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
==========================================
+ Coverage 85.33% 85.35% +0.02%
==========================================
Files 113 113
Lines 12802 12802
==========================================
+ Hits 10924 10927 +3
+ Misses 1878 1875 -3 ☔ View full report in Codecov by Sentry. |
Also switch to inbuilt filename encode helper, since it accepts paths
6d0dc4a
to
fe8d087
Compare
Added in fe8d087, and it's a good thing you suggested it, because the test failure also prompted me to fix implicit filename handling a few lines down. |
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.
Thank you for adding the unit test 💯 I left one nit comment
I was able to test this out manually with the following and it works as expected 🟢
import os
from pathlib import Path
from slack_sdk import WebClient
client = WebClient(
token=os.environ.get("SLACK_BOT_TOKEN"),
)
current_dir = os.path.dirname(__file__)
file = Path(f"{current_dir}/data/slack_logo.png")
upload = client.files_upload_v2(
channel="C1234",
title="Good Old Slack Logo",
filename="slack_logo.png",
file=file,
)
But I think most static type checkers like mypy
will reject this with the following error
error: Argument "file" to "files_upload_v2" of "WebClient" has incompatible type "Path"; expected "str | bytes | IOBase | None" [arg-type]
We would need to
- Update the type of the
file
argument inWebClient.files_upload_v2
- run
python scripts/codegen.py
to propagate the changes toWebClient
,AsyncWebClient
andLegacyWebClient
- Write proper unit tests for those changes
If you do not want to do these changes we can merge this PR as is and I can follow up with a PR for type support
@@ -101,6 +102,13 @@ def test_files_upload_v2_issue_1356(self): | |||
file_io_item = _to_v2_file_upload_item({"file": file_io, "filename": "foo.txt"}) | |||
assert file_io_item.get("filename") == "foo.txt" | |||
|
|||
def test_files_upload_v2_paths(self): |
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: maybe this could be name this something more descriptive like
def test_files_upload_v2_paths(self): | |
def test_to_v2_file_upload_item_can_accept_file_as_path(self): |
Summary
Allow passing Path objects into file upload functions.
Testing
Attempt to pass a
Path
object to one of the file upload functions that takes a string path. Also pass a string path to ensure there are no regressions.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.sh
after making the changes.I did get S3 test failures with the latter (without my change), FYI.