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. |
brettcannon
left a comment
There was a problem hiding this comment.
One minor thing, otherwise LGTM!
| if extract_dir is None: | ||
| extract_dir = os.getcwd() | ||
|
|
||
| filename = os.fspath(filename) |
There was a problem hiding this comment.
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.
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.