Skip to content
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

Adjust shutil.move stubs to allow bytes/Pathlike[bytes] args #6832

Merged
merged 2 commits into from
Jan 8, 2022

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Jan 5, 2022

Closes #6831. Thorough example included in the issue.

TLDR: shutil.move can take bytes arguments


else:
# See https://bugs.python.org/issue32689
def move(src: str, dst: StrPath, copy_function: _CopyFn = ...) -> _PathReturn: ...
def move(src: str | bytes, dst: StrOrBytesPath, copy_function: _CopyFn = ...) -> _PathReturn: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the linked bug applies to Pathlike[bytes] as well, but I've verified it works fine with bytes for Python < 3.9.

>>> import shutil
>>> shutil.move(b"test1.txt", b"test2.txt")
'test2.txt'

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Jan 5, 2022

I don't think mixing str and bytes paths works. The source code has this:

real_dst = os.path.join(dst, _basename(src))

which fails if it tries to join strings and bytes.

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 5, 2022

I don't think mixing str and bytes paths works. The source code has this:

real_dst = os.path.join(dst, _basename(src))

which fails if it tries to join strings and bytes.

This def works for me:

>>> import shutil
>>> shutil.move("test.txt", b"test2.txt")
b'test2.txt'
>>> shutil.move(b"test2.txt", "test.txt")
'test.txt'
>>> shutil.move("some_folder", "some_folder2")
'some_folder2'
>>> shutil.move(b"some_folder2", b"some_folder")
b'some_folder'
>>> shutil.move(b"some_folder", "some_folder2")
'some_folder2'
>>> shutil.move("some_folder2", b"some_folder")
b'some_folder'

EDIT: I noticed that bit in the source code only impacts directories, so I've added them above. They still seem to work though.

@Akuli
Copy link
Collaborator

Akuli commented Jan 5, 2022

The code I mentioned runs when dst is a directory.

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 5, 2022

The code I mentioned runs when dst is a directory.

Ahh it's only when dst is an existing directory.

In that case yes I get errors:

>>> shutil.move("some_folder", b"some_existing_folder")
...
TypeError: Can't mix strings and bytes in path components
>>> shutil.move(b"some_folder", "some_exising_folder")
...
File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 771, in _basename
    return os.path.basename(path.rstrip(sep))
TypeError: a bytes-like object is required, not 'str'

Feels like a bug given that it's only in this section of the code...

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 5, 2022

The second of those issues seems like a bigger issue since this would also fail:

>>> shutil.move(b"some_folder", b"some_exising_folder")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 810, in move
    real_dst = os.path.join(dst, _basename(src))
  File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/shutil.py", line 771, in _basename
    return os.path.basename(path.rstrip(sep))
TypeError: a bytes-like object is required, not 'str'

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 5, 2022

With those test cases, I think we should close this PR since bytes will always break the existing folder case.

Do you happen to know where the best place flag this issue is for cpython?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 5, 2022

Edit: disregard my comment, I just pattern matched to a thing I'd seen before (shutil has historically not supported path objects well, in general worth double checking these kinds of changes against older Pythons)

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 6, 2022

No worries, it looks like the types shutil functions accept is a bit inconsistent in general (although looks like they're improving) 😄

I'm working through the other shutil functions atm as I'm finding a lot of them work fine with bytes. Once I've gone through all the others we should hopefully have quite a strong precedent for getting move updated to accept bytes arguments in the existing dst case.

In the meantime, maybe it's worth me removing my changes and just adding a comment above the move stub mentioning the reasoning for not accepting bytes argument (or a link to this PR) so people are aware of our reasoning for not changing it (given that from my examples its easy to be misled by this function)?

@JelleZijlstra
Copy link
Member

In the meantime, maybe it's worth me removing my changes and just adding a comment above the move stub mentioning the reasoning for not accepting bytes argument (or a link to this PR) so people are aware of our reasoning for not changing it (given that from my examples its easy to be misled by this function)?

That seems right to me.

@jpy-git
Copy link
Contributor Author

jpy-git commented Jan 7, 2022

I've reverted the typing changes and added a comment linking to this PR to highlight for future contributors 👍

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Akuli Akuli merged commit 0e5a00b into python:master Jan 8, 2022
@Akuli
Copy link
Collaborator

Akuli commented Jan 8, 2022

Do you happen to know where the best place flag this issue is for cpython?

bugs.python.org. My experience with reporting and fixing bugs in cpython hasn't been great, but you might have better luck.

eaftan added a commit to eaftan/python-dotenv that referenced this pull request Jan 10, 2023
These paths can flow into `shutil.move`, which does not accept byte
paths or (int) file descriptors.  See python/typeshed#6832
theskumar pushed a commit to theskumar/python-dotenv that referenced this pull request Jan 11, 2023
* Fix type hint for load_dotenv

Fixes #431

* Quote type hints to avoid runtime errors in earlier Python versions

* Revise type of dotenv_path parameter

Based on PR feedback and typeshed's type hint for the built-in open()
function:
https://github.com/python/typeshed/blob/e2d67bf7034f68c07bd35150247e58e0817725d9/stdlib/builtins.pyi#L1421

* Allow only string paths, not byte paths

These paths can flow into `shutil.move`, which does not accept byte
paths or (int) file descriptors.  See python/typeshed#6832

* Create a type alias for the paths this library accepts

And use it consistently in main.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shutil.move stub does not account for bytes paths
4 participants