Reject invalid token arguments in Version.bump_prerelease and Version.bump_build - #483
Conversation
…ation Define each prerelease and build identifier pattern once and interpolate it into _REGEX_TEMPLATE, so the parser and bump token validation cannot drift apart. Compiled patterns are unchanged in behaviour. Make test_bump_methods_still_accept_empty_and_none assert the exact result for "" and None instead of membership in a tuple of outcomes. Co-Authored-By: Claude Opus 5 <[email protected]>
tomschr
left a comment
tomschr
left a comment
There was a problem hiding this comment.
Thank you @Str0k for your contribution! 👍
I have some suggestions for you to review. It's about avoiding too many class variables and using the .pattern property from a pattern object.
Let me know what you think! Overall, great work!
|
Thanks @tomschr, that is simpler. Applied in af07f02.
The full suite passes (438 passed, 1 skipped, 100% coverage). I also compared the old and new versions on 100,028 deterministic inputs for each of the four affected patterns; matches and captured groups were identical. |
|
Thank you @Str0k, that looks great now! 👍 I'm ready to merge it. |
2 participants
Calling Version.parse("1.0.0").bump_build("build+meta") or bump_prerelease("rc.") previously returned a Version object whose string representation ("1.0.0+build+meta.1", "1.0.0-rc..1") is not valid SemVer and cannot be round-tripped through Version.parse(). After this fix, both methods validate the dot-separated token against the SemVer identifier grammar used by the parser and raise ValueError for malformed tokens such as "bu/ild", "build+meta", "build..2", "rc." or a numeric prerelease segment with leading zeros ("rc.05"). Valid tokens ("build", "rc.1", "dev-1", "alpha.0") and the documented None/empty-string defaults are unchanged.
The parser (Version.parse/is_valid) enforces the SemVer 2.0.0 identifier grammar, but the bump methods concatenated the raw token string without any check, so the library could produce Version objects it would itself reject on the next parse. The fix adds a small _validate_identifiers classmethod that splits the token on '.' and fullmatches each part against the same identifier regexes the parser template uses for prerelease (numeric identifiers without leading zeros, or alphanumeric identifiers) and build ([0-9A-Za-z-]+) parts, and calls it from the explicit-token branch of both bump methods. This keeps the validation rules in one place and leaves the string parser and all other behavior untouched.
Validation:
AI assistance: implementation and independent review used Hermes with GLM 5.3. Test evidence was reproduced in clean checkouts. This does not represent a human review.
An additional model reviewed the supplied patch and evidence without executing tools.
Changelog: the reviewed bugfix entry is attached using this PR number after draft creation, following changelog.d/README.rst. The PR is marked ready only after the fragment passes Towncrier.