fix: flexible bundle regex, correct error messages, remove duplicate definition#327
Open
maksimtech wants to merge 1 commit intovitiko98:masterfrom
Open
fix: flexible bundle regex, correct error messages, remove duplicate definition#327maksimtech wants to merge 1 commit intovitiko98:masterfrom
maksimtech wants to merge 1 commit intovitiko98:masterfrom
Conversation
…definition Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
mzilinski
reviewed
Mar 31, 2026
mzilinski
left a comment
There was a problem hiding this comment.
Review PR #327
Gutes, fokussiertes Fix — alle vier Änderungen sind sinnvoll und korrekt. Hier mein Feedback:
✅ Positiv
- Flexible Regex:
\d+statt\d{3}/\d{9}/\w{32}— macht das Matching robuster gegen zukünftige Formatänderungen bei Qobuz. Guter defensiver Ansatz. - Fehlermeldung korrigiert:
NotImplementedError("Bundle URL found")war offensichtlich falsch (Nachricht sagte "found" obwohl "not found" gemeint war).ValueError("Bundle URL not found")ist sowohl semantisch korrekt als auch der richtige Exception-Typ. - Doppelte Definition entfernt:
_BUNDLE_URL_REGEXwar doppelt definiert (Zeilen 20-22 und 25-27). Die zweite hat die erste still überschrieben — gut, dass das aufgeräumt wurde.
💬 Anmerkung
- Zeile 53 hat noch eine ähnliche
NotImplementedError("Failed to match APP ID")— könnte man ebenfalls zuValueErrorändern, daNotImplementedErroreigentlich für abstrakte Methoden gedacht ist. Das ist aber out-of-scope für diesen PR.
Fazit
Sauber, minimal, korrekt. 👍
Author
|
Thank you for the thorough review! |
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.
Problem
Bundle.__init__fails silently when Qobuz ships a bundle whose version suffix doesn't match the hardcoded pattern, then raises a completely misleading error that hides the real cause.Root cause — overly strict
_BUNDLE_URL_REGEXIf the suffix format ever changes (e.g.
8.2.0-b1234), the regex returns no match and the code raises:This misleading message makes the failure almost impossible to diagnose.
Changes
bundle.py[a-z]\d{3}→[a-z]\d+in_BUNDLE_URL_REGEX(one-or-more digits)bundle.py\d{9}→\d+and\w{32}→\w+in_APP_ID_REGEX(flexible length)bundle.pyNotImplementedError("Bundle URL found")→ValueError("Bundle URL not found")bundle.py_BUNDLE_URL_REGEXdefinition (was defined twice, lines 20–22 and 25–27)Verification
Tested against the live bundle (
/resources/8.1.0-b019/bundle.js) — all three patterns match correctly andget_secrets()returns valid decoded secrets.🤖 Generated with Claude Code