FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-149306: Improve error messages in wave module to include the offending value by htjworld · Pull Request #149307 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-149306: Improve error messages in wave module to include the offending value - #149307

Open
htjworld wants to merge 3 commits into
python:mainfrom
htjworld:gh-149306-wave-error-messages
ConversationCommits (3)ChecksFiles changed
Open

gh-149306: Improve error messages in wave module to include the offending value#149307
htjworld wants to merge 3 commits into
python:mainfrom
htjworld:gh-149306-wave-error-messages

Conversation

htjworld commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Include the offending value in wave.Error messages raised by:

  • Wave_read._read_fmt_chunk (parses WAV file header)
  • Wave_write.setnchannels
  • Wave_write.setsampwidth
  • Wave_write.setframerate

Before:

wave.Error: bad # of channels
wave.Error: bad sample width
wave.Error: bad frame rate

After:

wave.Error: bad # of channels: 0
wave.Error: bad sample width: 0
wave.Error: bad frame rate: -1

Motivation

Wave_read: Values come from the WAV file header — they never appear
in the traceback. The error message is the only debugging clue.

Wave_write: When values originate from variables, configuration files,
or parsed data rather than literals, the traceback shows the call site but
not the value itself. For example, w.setframerate(config['rate']) raises
bad frame rate with no indication of what config['rate'] was.

This extends the clear-feedback approach introduced in gh-144976, which fixed
the validation order in setframerate to raise immediately at the point of
the mistake. Showing the offending value is the natural next step.

For setframerate, the original value (before int(round())) is shown rather
than the rounded result, so callers see exactly what they passed.

Precedent

Notes

  • Error type is unchanged; only the message text is refined.
  • The bits/bytes unit ambiguity in bad sample width predates this PR and can be addressed separately.
  • NaN/Inf handling in setframerate is pre-existing behaviour and out of scope here.

pythongh-149306: Improve error messages in wave module to include the…
d1ba708 
… offending value

Include the offending value in wave.Error messages raised by
Wave_read._read_fmt_chunk and Wave_write setters (setnchannels,
setsampwidth, setframerate). For setframerate, the original value is
shown rather than the rounded one to aid debugging.
bedevere-app Bot mentioned this pull request May 3, 2026
Open
htjworld force-pushed the gh-149306-wave-error-messages branch 2 times, most recently from cc28830 to d1ba708 Compare May 3, 2026 02:14
picnixz requested changes May 3, 2026
View reviewed changes
Comment thread Lib/wave.py Outdated
def _read_fmt_chunk(self, chunk):
try:
self._format, self._nchannels, self._framerate, dwAvgBytesPerSec, wBlockAlign = struct.unpack_from('<HHLLH', chunk.read(14))
self._format, nchannels, self._framerate, dwAvgBytesPerSec, wBlockAlign = struct.unpack_from('<HHLLH', chunk.read(14))

picnixz May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Don't change this. It is a breaking change. Previously the attribute would have been already set after passing unpack_from but now it will be deferred.

Comment thread Lib/wave.py Outdated
Comment on lines +525 to +528
rounded = int(round(framerate))
if rounded <= 0:
raise Error(f'bad frame rate: {framerate!r}')
self._framerate = rounded

picnixz May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
rounded = int(round(framerate))
if rounded <= 0:
raise Error(f'bad frame rate: {framerate!r}')
self._framerate = rounded
rounded_framerate = int(round(framerate))
if rounded_framerate <= 0:
raise Error(f'bad frame rate: {framerate!r}')
self._framerate = rounded_framerate

Comment thread Lib/test/test_wave.py Outdated
Comment on lines +437 to +438
with self.assertRaisesRegex(wave.Error,
re.escape(f'bad # of channels: {nchannels!r}')):

picnixz May 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For readability purposes, can you put the message outside?

message = re.escape(...)
with wave.open(...):
   with self.assertRaisesRegex(..., err):
        f.setchannels(channels)

bedevere-app Bot commented May 3, 2026

Copy link
Copy Markdown

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.

htjworld commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

  • Reverted _read_fmt_chunk to assign directly to self._nchannels (no temporary variable)
  • Renamed rounded to rounded_framerate
  • Extracted assertRaisesRegex messages to variables for readability

bedevere-app Bot commented May 3, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

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

bedevere-app Bot requested a review from picnixz May 3, 2026 12:02

read-the-docs-community Bot commented May 3, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cpython-previews | 🛠️ Build #32514806 | 📁 Comparing 2d4c5e4 against main (836fbda)

  🔍 Preview build  

8 files changed · ± 8 modified

± Modified

htjworld commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

Hi @picnixz, gentle ping! All review comments have been addressed. Please let me know if anything else is needed. Thanks!

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 30, 2026

htjworld commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Hi @picnixz, this went stale but the requested changes are still in from May 3: reverted the _read_fmt_chunk assignment, renamed to rounded_framerate, extracted the assertRaisesRegex messages. CI is green. Let me know if you want anything else changed.

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Sep 5, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

picnixz Awaiting requested review from picnixz

Assignees

No one assigned

Labels

awaiting change review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL