bpo-31412: wave.open takes a path-like object by mscuthbert · Pull Request #3484 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation41 Commits10 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Choose a reason for hiding this comment

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

I'd also check for a write method here.

Choose a reason for hiding this comment

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

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().

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)}')

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

PathLike -> :class:`os.PathLike`

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

This change is out of scope for this issue.

Choose a reason for hiding this comment

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

Please revert this change.

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

def test_open(self):

Choose a reason for hiding this comment

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

I'd rename this to something like test_open_pathlike.

Choose a reason for hiding this comment

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

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

Test not only reading, but writing too.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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)}')

Choose a reason for hiding this comment

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

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

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)}')

Choose a reason for hiding this comment

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

"open filehandle" -> file-like object

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.

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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)}')

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@@ -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`.

Choose a reason for hiding this comment

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

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

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:

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

def test_open(self):

Choose a reason for hiding this comment

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

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

Test not only reading, but writing too.

@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?

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

I have made the requested changes; please review again

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

@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.

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.

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

~~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.

@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.

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.