Fix three error-handling correctness issues (mask_func, QRData write, best_fit)#425
Open
nssmd wants to merge 1 commit intolincolnloop:mainfrom
Open
Fix three error-handling correctness issues (mask_func, QRData write, best_fit)#425nssmd wants to merge 1 commit intolincolnloop:mainfrom
nssmd wants to merge 1 commit intolincolnloop:mainfrom
Conversation
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
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.
util.py:161TypeError: can only concatenate str (not 'int') to strforpattern=8. The intended message is hidden behind the concatenation.ValueError(the type is correct; the value is not). Tracks the case described in issue #191.util.py:455-463(QRData.write)QRData(b"a", mode=ALPHA_NUM, check_data=False).write(buf)silently produces an undecodable QR becauseALPHA_NUM.find('a') == -1andBitBuffer.put(-1, 6)writes six1-bits.find()'s return value in the writer and raiseValueErrornaming the offending byte(s).main.py:223-227(best_fit)bisect_leftreturns41. Assigning41toself.versiontriggers the setter, which callscheck_versionand raisesValueError("Invalid version (was 41…)")— preventing the library's ownDataOverflowErrorfrom being raised.new_version, raiseDataOverflowErroron41, then assign. Same observable behavior on success; semantically correct exception on overflow.Why this matters
Two of the three fixes change exception type, not just text. Callers
that wrap
qr.make()and dispatch onDataOverflowError(e.g. to askthe 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")willsilently 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:
TestMaskFuncErrorMessage::test_mask_func_invalid_pattern_messageTestQRDataAlphaNumWithNonAlphaChar::test_alphanum_with_invalid_char_raisesTestDataOverflow::test_overflow_when_autofit_exceeds_v40Backward 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
ValueErrorfromQRData.writeis only reachable via theopt-in
check_data=Falseconstructor 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
TypeErrorandonly 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.pyandmain.pylookingfor three patterns: (a) error messages built with
+instead off"", (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 aside run of the OpenAI Codex CLI which observed the same
ValueError("Invalid version (was 41, expected 1 to 40)")from aclean 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.