Fix three error-handling correctness issues (mask_func, QRData write, best_fit) - #425
Open
nssmd wants to merge 1 commit into
Open
Fix three error-handling correctness issues (mask_func, QRData write, best_fit)#425nssmd wants to merge 1 commit into
nssmd wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
1. mask_func: replace `"Bad mask pattern: " + pattern` (which raises TypeError on str+int when pattern is non-int) with an f-string and raise ValueError, since for an out-of-range integer the type is fine but the value is not. Closes the issue tracked in lincolnloop#191. 2. QRData.write: validate ALPHA_NUM characters in the write path. With `check_data=False`, ALPHA_NUM.find() returned -1 for any character outside the table; BitBuffer.put(-1, n) silently writes all-ones bits, producing a QR that no compliant scanner can decode. Now raises a ValueError explaining which character is invalid. 3. best_fit: compute version into a local first, then raise DataOverflowError on 41 *before* assigning to the property setter. Previously the setter's check_version() raised ValueError, masking the library's own DataOverflowError exception. All three are reachable via the public API and are covered by new regression tests under test_deliverables/test_blackbox.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: Fix three error-handling correctness issues
Summary
While building an end-to-end test suite for the library, three latent
correctness issues surfaced. All three are reachable from the public
API and lead to misleading or wrong-typed exceptions. The patch fixes
the failure modes and the test suite gains regression tests for each.
Why this matters
Two of the three fixes change exception type, not just text. Callers
that wrap qr.make() and dispatch on DataOverflowError (e.g. to ask
the user to shorten the payload) currently cannot catch the overflow
case via that exception, because it's intercepted by the
property-setter validation. Same story with mask_func: defensive code
that asserts pytest.raises(TypeError, match="Bad mask pattern") will
silently fail to match because the prefix never gets emitted. These
are user-facing contract regressions even when the bug count looks
small.
Verification
Three unit tests assert the new correct behavior:
Backward compatibility
The exception types change for the three failure modes above
(BUG #1: TypeError → ValueError; BUG #3: ValueError →
DataOverflowError). For success paths there is no observable change.
The new ValueError from QRData.write is only reachable via the
opt-in check_data=False constructor flag with non-ALPHA_NUM data —
i.e., previously broken usage — so no existing valid program is
affected.
If you'd prefer to preserve the exact exception class for #1 to keep
strict back-compat, I can change the patch to keep TypeError and
only fix the message-construction crash. Happy to iterate.
Discovery process
These were found while writing a black-box / white-box test plan
against v8.2 as part of a software-testing course assignment. The
suspects were short-listed by reading util.py and main.py looking
for three patterns: (a) error messages built with + instead of
f"", (b) public methods with an opt-in skip-validation flag, and
(c) numeric overflow paths that route through a setter. All three
were independently reproduced via pytest, and #3 was confirmed by a
side run of the OpenAI Codex CLI which observed the same
ValueError("Invalid version (was 41, expected 1 to 40)") from a
clean checkout.
Discovered & patched by the python-qrcode testing study group, with
the assistance of Claude Opus 4.7 (Anthropic) for static review and
Codex CLI for cross-verification.