Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
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 ourterms of service andprivacy 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
python-cla-botbot commentedApr 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb 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 |
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 thecurrent value itself, and if it's not yet set, they should wrap it in atry-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:
ifself.has_data:assertself.getnchannels()==EXPECT_NCHANNELSelse: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 usesynthesize_stream_raw() or to have amerge_and_synthesize function which
forsinpiper.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`, ifthe value is set after data was written. | |
| Raise:exc:`wave.Error` ifa 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` isnotraised, if the valueis the same. | |
| ..versionchanged::next | |
| No:exc:`wave.Error` is raised if the valuedoes 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.
Theint(round(framerate)) should be tested when two distinct input framerates give the same rounded framerate.
| raise Error('cannot change parameters after starting to write') | ||
| ifframerate <= 0: | ||
| ifrounded_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 writeself.setframerate(0.5) and the framerate would be set to 0. With this change, we prevent a bad framerate.
So also test thatself.setframerate(0.5) raises an exception, but notself.setframerate(0.51).
Uh oh!
There was an error while loading.Please reload this page.
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.