-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
base: main
Are you sure you want to change the base?
Conversation
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 |
b8f4980
to
c55c503
Compare
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 |
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'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.
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 |
c55c503
to
4fd6c81
Compare
4fd6c81
to
8b77c5a
Compare
I have made the requested changes; please review again. |
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
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.
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. |
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.
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. |
.. versionchanged:: 3.14 | ||
:exc:`wave.Error` is not raised, if the value is the same. |
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.
.. 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.
writer.setnchannels(2) | ||
writer.setsampwidth(2) | ||
writer.setframerate(2) |
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.
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)) |
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.
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: |
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 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)
.
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.