bpo-31412: wave.open takes a path-like object #3484

mscuthbert · 2017-09-10T19:38:35Z

https://bugs.python.org/issue31412

berkerpeksag · 2017-11-01T08:01:27Z

Lib/wave.py

+    try:
+        f = os.fspath(f)
+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):


I'd also check for a write method here.

I think it is enough to check for a read in Wave_read and for a write on Wave_write. close should be optional.

Note that in Wave_read and Wave_write constructors there are checks isinstance(f, str). This means that bytes patch are not supported, and file descriptors are not supported. I would use hasattr(f, 'read') or hasattr(f, 'write') instead and just pass other objects to builtins.open().

berkerpeksag · 2017-11-01T08:02:38Z

Lib/wave.py

+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):
+            raise TypeError('open() takes str, bytes, a PathLike object, '
+                            + f'or an open filehandle, not {type(f)}')


{type(f).__name__!r} would give us a more readable name.

Actually bytes are not supported with current implementation. See the comment above.

Misc/NEWS.d/next/Library/2017-09-10-15-20-14.bpo-31412.lNbm-A.rst

berkerpeksag · 2017-11-01T08:04:54Z

Misc/NEWS.d/next/Library/2017-09-10-15-20-14.bpo-31412.lNbm-A.rst

@@ -0,0 +1,2 @@
+`wave.open()` now accepts PathLike objects. Contributed by Michael Scott


PathLike -> :class:`os.PathLike`

Or just path-like. Or :term:'path-like objects <path-like object>' (use backquotes instead of apostrophes).

Misc/NEWS.d/next/Library/2017-09-10-15-20-14.bpo-31412.lNbm-A.rst

berkerpeksag · 2017-11-01T08:08:12Z

Lib/wave.py

@@ -1,13 +1,14 @@
-"""Stuff to parse WAVE files.
+"""Module for parsing WAVE audio files.


This change is out of scope for this issue.

berkerpeksag · 2017-11-01T08:08:27Z

Lib/test/test_wave.py

 import sys
 import wave

-


Please revert this change.

berkerpeksag · 2017-11-01T08:11:03Z

Lib/test/test_wave.py

@@ -108,6 +110,26 @@ def test__all__(self):
        blacklist = {'WAVE_FORMAT_PCM'}
        support.check__all__(self, wave, blacklist=blacklist)

+    def test_open(self):


I'd rename this to something like test_open_pathlike.

This test checks only failed cases. It would be better to test successful opens.

Test not only reading, but writing too.

Is it acceptable to add a wave file to the test directory? I didn't test on a .wav file since even a small one can be 20-30k -- if that's okay then I will add one and test that.

There are sample wave files in the audiodata directory. See how other tests use them.

berkerpeksag · 2017-11-01T08:11:41Z

Lib/wave.py

+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):
+            raise TypeError('open() takes str, bytes, a PathLike object, '
+                            + f'or an open filehandle, not {type(f)}')


I think we also need to add from None to make the traceback clearer.

berkerpeksag · 2017-11-01T08:12:59Z

Lib/wave.py

+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):
+            raise TypeError('open() takes str, bytes, a PathLike object, '
+                            + f'or an open filehandle, not {type(f)}')


"open filehandle" -> file-like object

bedevere-bot · 2017-11-01T08:13:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka

If change the wave module, the aifc and sunau modules should be changed too. All three module should have the same interface.

serhiy-storchaka · 2017-11-01T08:44:34Z

Lib/wave.py

+    try:
+        f = os.fspath(f)
+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):


I think it is enough to check for a read in Wave_read and for a write on Wave_write. close should be optional.

Note that in Wave_read and Wave_write constructors there are checks isinstance(f, str). This means that bytes patch are not supported, and file descriptors are not supported. I would use hasattr(f, 'read') or hasattr(f, 'write') instead and just pass other objects to builtins.open().

serhiy-storchaka · 2017-11-01T08:46:32Z

Lib/wave.py

+    except TypeError:
+        if not hasattr(f, 'read') or not hasattr(f, 'close'):
+            raise TypeError('open() takes str, bytes, a PathLike object, '
+                            + f'or an open filehandle, not {type(f)}')


Actually bytes are not supported with current implementation. See the comment above.

serhiy-storchaka · 2017-11-01T08:49:23Z

Misc/NEWS.d/next/Library/2017-09-10-15-20-14.bpo-31412.lNbm-A.rst

@@ -0,0 +1,2 @@
+`wave.open()` now accepts PathLike objects. Contributed by Michael Scott


Or just path-like. Or :term:'path-like objects <path-like object>' (use backquotes instead of apostrophes).

serhiy-storchaka · 2017-11-01T08:50:53Z

Doc/library/wave.rst

@@ -47,6 +47,9 @@ The :mod:`wave` module defines the following function and exception:
   .. versionchanged:: 3.4
      Added support for unseekable files.

+   .. versionchanged:: 3.7
+      Added support for :term:`path-like object`.


Or :term:'path-like objects <path-like object>' (use backquotes instead of apostrophes).

serhiy-storchaka · 2017-11-01T08:52:53Z

Doc/library/wave.rst

-   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 :term:`path-like object`, open the file
+   by that name, otherwise treat it as a file-like object.  *mode* can be:


Maybe add a reference to the "file-like object" term?

I looked in the glossary and "file-like object" redirects to "file object" so I used that term instead. Hope that that's okay.

serhiy-storchaka · 2017-11-01T08:57:04Z

Lib/test/test_wave.py

@@ -108,6 +110,26 @@ def test__all__(self):
        blacklist = {'WAVE_FORMAT_PCM'}
        support.check__all__(self, wave, blacklist=blacklist)

+    def test_open(self):


This test checks only failed cases. It would be better to test successful opens.

Test not only reading, but writing too.

mscuthbert · 2017-11-01T20:11:59Z

@serhiy-storchaka -- thanks for the comments -- would patching aifc and sunau be in the scope for this PR or open a separate issue for them?

serhiy-storchaka · 2017-11-01T20:34:08Z

I think it is better to do this in one issue. These modules have much common and use common tests.

will fix the other two before requesting review again

mscuthbert · 2017-11-28T22:41:58Z

I have made the requested changes; please review again

bedevere-bot · 2017-11-28T22:42:01Z

Thanks for making the requested changes!

@serhiy-storchaka, @berkerpeksag: please review the changes made to this pull request.

Doc/library/sunau.rst

Doc/library/aifc.rst

bedevere-bot · 2019-04-08T07:59:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

csabella · 2020-02-07T12:23:59Z

@mscuthbert, please resolve the merge conflicts and address the code review comments.

mscuthbert · 2020-02-11T02:48:15Z

@mscuthbert, please resolve the merge conflicts and address the code review comments.

Hi @csabella -- I will do the merge conflicts soon, but I don't believe there were any code review comments -- my last correspondence was waiting for a review of the other changes. Thanks for taking the time to look at this again.

Doc/library/wave.rst

jacobtylerwalls · 2021-06-14T16:57:16Z

I don't believe there were any code review comments

Hi @mscuthbert , I was filtering on audio issues and stumbled across this. I've gathered "awaiting changes" means "waiting for author", and it was added when the versionchanged annotations were asked to be bumped; I bet you could bump all the way to 3.11 and then say the magic words to get the label removed and then you'd be more likely to get a substantive review based on having the right labels.

github-actions · 2022-02-21T00:08:39Z

This PR is stale because it has been open for 30 days with no activity.

hugovk · 2022-04-11T10:34:28Z

Note the aifc and sunau modules are deprecated in 3.11 and set for removal in 3.13, so maybe those changes could be dropped? The wave module is being kept.

See PEP 594 – Removing dead batteries from the standard library and #91217.

methane · 2022-04-12T03:03:02Z

~~Those modules shares most tests. So I prefer to change them all, althoguh aifc and sunau are deprecated.~~

I was wrong. AudioMiscTest had removed already. I agree that we need to fix only wave.

AA-Turner · 2022-06-07T12:27:02Z

Reopening per #75593 (comment)

A

AA-Turner · 2022-06-07T12:32:54Z

@mscuthbert please can you merge master?

A

mscuthbert · 2022-06-09T21:05:03Z

@mscuthbert please can you merge master?

A

Unfortunately, the fork of the branch got severely corrupted (I had not kept the master/main rename) and the new fork of the same name isn't being seen as related by Github, so after trying to do a merge from these commits for an hour, I wasn't able to make progress. Probably the issue should be closed.

Because the audiotests look at all three of the audio modules, there would need to be more editing in the test files to remove the changes to sunau and aifc and still make the tests pass.

AA-Turner · 2022-06-09T21:22:57Z

OK, I will close this PR and mark the issue as pending -- if you manage to get things working please ping me and I can review/update things as appropriate.

A

arhadthedev · 2023-03-04T18:28:09Z

Superseded by more general gh-102425.

mscuthbert added 5 commits September 7, 2017 00:00

Merge remote-tracking branch 'python/master'

f8fd6f7

Merge remote-tracking branch 'python/master'

dd997b7

Merge remote-tracking branch 'python/master'

83ecd48

Merge remote-tracking branch 'python/master'

d4e19dd

bpo-31412: wave.open accepts a path-like object

51aa76a

bedevere-bot added the awaiting review label Sep 10, 2017

the-knights-who-say-ni added the CLA signed label Sep 10, 2017

berkerpeksag requested changes Nov 1, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 1, 2017

serhiy-storchaka requested changes Nov 1, 2017

View reviewed changes

mscuthbert added 3 commits November 28, 2017 16:11

Merge remote-tracking branch 'python/master' into fix-issue-31412

5e5bf41

intermediate commit fixed wave module

189c4e2

will fix the other two before requesting review again

Added sunau and aifc to scope

e56e644

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 28, 2017

Update wave.rst

e2cea59

methane requested changes Apr 8, 2019

View reviewed changes

Doc/library/sunau.rst Outdated Show resolved Hide resolved

Doc/library/aifc.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting change review labels Apr 8, 2019

methane reviewed Feb 12, 2020

View reviewed changes

Doc/library/wave.rst Outdated Show resolved Hide resolved

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 21, 2022

Apply suggestions from code review

4a82d65

mscuthbert mannequin mentioned this pull request Apr 12, 2022

wave.open does not accept PathLike objects #75593

Closed

AA-Turner reopened this Jun 7, 2022

AA-Turner added type-feature A feature request or enhancement stdlib Python modules in the Lib dir and removed CLA signed stale Stale PR or inactive for long period of time. labels Jun 7, 2022

AA-Turner closed this Jun 9, 2022

arhadthedev mentioned this pull request Mar 4, 2023

All standard library functions accepting str paths must accept PathLike as well #102425

Open

		@@ -0,0 +1,2 @@
		`wave.open()` now accepts PathLike objects. Contributed by Michael Scott

		@@ -1,13 +1,14 @@
		"""Stuff to parse WAVE files.
		"""Module for parsing WAVE audio files.

bpo-31412: wave.open takes a path-like object #3484

bpo-31412: wave.open takes a path-like object #3484

Conversation

mscuthbert commented Sep 10, 2017 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 1, 2017

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscuthbert commented Nov 1, 2017

serhiy-storchaka commented Nov 1, 2017

mscuthbert commented Nov 28, 2017

bedevere-bot commented Nov 28, 2017

bedevere-bot commented Apr 8, 2019

csabella commented Feb 7, 2020

mscuthbert commented Feb 11, 2020

jacobtylerwalls commented Jun 14, 2021

github-actions bot commented Feb 21, 2022

hugovk commented Apr 11, 2022

methane commented Apr 12, 2022 • edited Loading

AA-Turner commented Jun 7, 2022

AA-Turner commented Jun 7, 2022

mscuthbert commented Jun 9, 2022

AA-Turner commented Jun 9, 2022 • edited Loading

arhadthedev commented Mar 4, 2023

mscuthbert commented Sep 10, 2017 •

edited by bedevere-bot

Loading

methane commented Apr 12, 2022 •

edited

Loading

AA-Turner commented Jun 9, 2022 •

edited

Loading