The Wayback Machine - https://web.archive.org/web/20250523120827/https://github.com/python/cpython/pull/132448
Skip to content

GH-132445: Allowing to reset parameters of Wave_write #132448

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ygerlach
Copy link

@ygerlach ygerlach commented Apr 12, 2025

This allows to re-set a parameter of Wave_write to the same value without causing an Error. This allows to implemet repeated writes to a wav file without special care for the first write.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 12, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Apr 12, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Apr 23, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm still not convinced but in any case, if this feature is to be accepted, we need tests and a NEWS entry, as well as a doc change.

@bedevere-app
Copy link

bedevere-app bot commented Apr 24, 2025

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.

@ygerlach
Copy link
Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Apr 30, 2025

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from picnixz April 30, 2025 16:26
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

TBH, I am neutral on this addition. I can understand that it's annoying for people passing around wave objects and forcibly changing the parameters, but OTOH I still believe it's incorrect to forcibly reset a parameter even if it's the same. Users should instead check the current value itself, and if it's not yet set, they should wrap it in a try-except call.

To ease their life, we could however expose the number of bytes that were already written so that they don't need a try-except but just something like:

if self.has_data:
    assert self.getnchannels() == EXPECT_NCHANNELS
else:
	self.setnchannels(EXPECT_NCHANNELS)

It could be annoying but I think it's more an API misuse rather an API flaw. For your use case, I think the correct way to do it is to use synthesize_stream_raw() or to have a merge_and_synthesize function which

for s in piper.synthesize_stream_raw(...):
	wav_file.writeframes(s)

So, while I have reviewed your code, I don't really think we'll make the change in the stdlib itself. Another reason is that it makes maintainability harder (if we add more setters in the future, it could be annoying if some values are inter-dependent as well).

@serhiy-storchaka should decide whether or not we accept this change.

@@ -198,11 +198,20 @@ Wave_write Objects

Set the number of channels.

Raises an :exc:`wave.Error`, if the value is set after data was written.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Raises an :exc:`wave.Error`, if the value is set after data was written.
Raise :exc:`wave.Error` if a different value is set after data was written.

Comment on lines +203 to +204
.. versionchanged:: 3.14
:exc:`wave.Error` is not raised, if the value is the same.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionchanged:: 3.14
:exc:`wave.Error` is not raised, if the value is the same.
.. versionchanged:: next
No :exc:`wave.Error` is raised if the value does not change.

Same for the rest of the docs.

Comment on lines +174 to +176
writer.setnchannels(2)
writer.setsampwidth(2)
writer.setframerate(2)
Copy link
Member

Choose a reason for hiding this comment

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

We also need to test rounded values.

@@ -502,27 +502,28 @@ def getsampwidth(self):
return self._sampwidth

def setframerate(self, framerate):
if self._datawritten:
rounded_framerate = int(round(framerate))
Copy link
Member

Choose a reason for hiding this comment

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

The int(round(framerate)) should be tested when two distinct input framerates give the same rounded framerate.

raise Error('cannot change parameters after starting to write')
if framerate <= 0:
if rounded_framerate <= 0:
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that before this change we could write self.setframerate(0.5) and the framerate would be set to 0. With this change, we prevent a bad framerate.

So also test that self.setframerate(0.5) raises an exception, but not self.setframerate(0.51).

@picnixz picnixz requested a review from serhiy-storchaka May 9, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants