-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-75593: Allow opening of path-like files with wave.open
#140951
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
gh-75593: Allow opening of path-like files with wave.open
#140951
Conversation
wave.open
wave.openwave.open
|
|
Lib/wave.py
Outdated
| def __init__(self, f): | ||
| self._i_opened_the_file = None | ||
| if isinstance(f, str): | ||
| if isinstance(f, (str, os.PathLike)): |
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.
As was suggested in #75593 (comment), it is easy to add support of bytes.
Lib/test/test_wave.py
Outdated
| def test_open_pathlike(self): | ||
| """It is possible to use `wave.read` and `wave.write` with a path-like file""" | ||
| with tempfile.NamedTemporaryFile(delete_on_close=False) as fp: | ||
| WAV_FILE = pathlib.Path(fp.name) |
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.
Use test.support.os_helper.FakePath instead of pathlib.Path. It should support arbitrary Path-like objects.
Also, test with a Path-like object wrapping a bytes path: FakePath(os.fsencode(...)).
Doc/library/wave.rst
Outdated
| .. function:: open(file, mode=None) | ||
|
|
||
| If *file* is a string, open the file by that name, otherwise treat it as a | ||
| If *file* is a string or a :term:`path-like object`, open the file by that name, otherwise treat it as a |
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.
Too long line.
Add also a versionchanged directive.
Misc/NEWS.d/next/Library/2025-11-04-12-16-13.gh-issue-75593.EFVhKR.rst
Outdated
Show resolved
Hide resolved
…VhKR.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
- Use `FakePath` instead of `pathlib.Path`. - Add an extra test case for the bytes path case. - Format the doc and add `versionchanged`.
eb1f0ab to
ba5ed6f
Compare
| with tempfile.NamedTemporaryFile(delete_on_close=False) as fp: | ||
| cases = ( | ||
| FakePath(fp.name), | ||
| FakePath(os.fsencode(fp.name)), |
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.
Test also for os.fsencode(fp.name).
| self.assertIsNone(cm.unraisable) | ||
|
|
||
|
|
||
| class WaveOpen(unittest.TestCase): |
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.
Isn't there a place for this test in an existing class? You can also look in audiotests -- if this was added before removing aifc, the test should be added there.
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 had also considered adding the new tests to MiscTestCase or WaveLowLevelTest - I don't have a good reason to create a new class now that I think of it, except that perhaps they don't strictly belong in either of those places.
Just to clarify your point about adding the tests to audiotests.py - are you saying that the new tests should simply be moved there instead? Or do you mean that the existing tests in test_wave.py should inherit the new tests similar to the existing design?
My rationale for adding the tests to test_wave.py is I did feel like they belonged in test_wave.py and not audiotests.py since the classes in audiotests.py are inherited by the classes in test_wave.py to assert a range of things which felt redundant for the new tests to also perform.
Doc/library/wave.rst
Outdated
|
|
||
| If *file* is a string, open the file by that name, otherwise treat it as a | ||
| file-like object. *mode* can be: | ||
| If *file* is a string or a :term:`path-like object`, open the file by that name, |
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.
or a bytes object.
Doc/library/wave.rst
Outdated
| Added support for unseekable files. | ||
|
|
||
| .. versionchanged:: 3.15 | ||
| Added support for :term:`path-like objects <path-like object>`. |
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.
and bytes objects.
- More accurate documentation. - More complete test cases.
serhiy-storchaka
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.
LGTM. 👍
| @@ -0,0 +1 @@ | |||
| Add support of :term:`path-like objects <path-like object>` in :func:`wave.open`. | |||
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.
And bytes objects.
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.
Oh sorry for that again 😅. I’ll update later today.
|
Thank you for your contribution @mbyrnepr2. |
|
It was a pleasure and thanks for your time and guidance @serhiy-storchaka |
#75593