-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-30218: support PathLike in shutil.unpack_archive #1367
Conversation
@JelleZijlstra, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @hynek and @larryhastings to be potential reviewers. |
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.
One minor thing, otherwise LGTM!
Lib/shutil.py
Outdated
@@ -958,6 +958,8 @@ def unpack_archive(filename, extract_dir=None, format=None): | |||
if extract_dir is None: | |||
extract_dir = os.getcwd() | |||
|
|||
filename = os.fspath(filename) |
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 notice that the tests show extract_dir
working for a path-like object, but there's no explicit conversion here. If it's because something else explicitly covers that then please add a comment. Otherwise just explicitly do the os.fspath()
conversion for extract_dir
.
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 it works because it's only passed to other functions that already accept PathLikes. However, it is passed to a handler function that is specific to a file format and maybe some handlers don't accept PathLike, so I'll just add os.fspath
for it anyway.
Do the docs need updating to mention path-like support? |
Not sure about the docs; my thinking is that every function using a path should support PathLike in 3.6 and up, so there's no point in explicitly noting it. But I'm open to adding it if you prefer being explicit. |
I was asking more in case the docs specifically mention that the arguments are expected to be a string or something like |
Just realized I forgot to ask if you could you add an entry to Misc/NEWS for this, @JelleZijlstra ? And don't put the entry at the top of the section to minimize potential merge conflicts. |
Now I just have to decide if this a bugfix or not. 🤔 I don't remember off the top of my head if we decided that os.PathLike support was considered a bugfix or not. |
After discussing things on python-dev, this has been flagged as an enhancement, hence a need to at least add a Sorry for all the little edits on this, @JelleZijlstra . |
I think a documentation change is needed. We've updated the documentation even if the module was already (indirectly) accept path-like objects. |
No description provided.