Repository navigation
Annotate supported bytes inputs to Version.is_valid - #495
Conversation
c0b47fd to
9a06b7c
Compare
There was a problem hiding this comment.
Thank you @xiehuanyi for your contribution, much appreciated! 👍
It looks fine. The only change I'd suggest is a small test case that address the byte string case. Something like this:
def test_is_valid_accepts_bytes():
assert Version.is_valid(b"1.0.0") is True
assert Version.is_valid(b"foo") is False
assert Version.is_valid("1.0.0".encode("UTF-8")) is TrueThe first two lines cover the valid and invalid cases. The third is optional, but it makes the UTF-8 wording in the docstring explicit.
A bytes case that isn't valid UTF-8 would also be worth a look, since parse() calls .decode("UTF-8"), which raises UnicodeDecodeError, not ValueError. Then is_valid() would propagate the exception instead of returning False. That's existing behavior, but it's relevant to what the docstring promises, so I'd check it before merging.
Could you add that please? Thanks!
|
@tomschr Thanks for the review. The requested byte-string tests are in I closed the PR because I reached my repair time limit before completing remote verification and the PR description update. I should have explained the closure at the time. |
|
Thank you @xiehuanyi for the answers and the additional commit. They look fine. I just have this minor thing. I'll merge it once the minor thing is fixed. Again, thanks for your contribution! 🎉 ❤️ |
|
@tomschr Thanks for reopening this. The byte-string regression tests are already in I will leave the PR open for your review. As noted earlier, I have reached the repair-time limit for this contribution, so I cannot make additional edits under the current workflow; the existing patch remains available for you to adjust if useful. |
I already changed it. It was just a minor fix from That fixes all remaining issues and it's ready to be merged. |
Version.is_valid()already accepts UTF-8 encodedbytesthroughVersion.parse(), but itsstrannotation makes type checkers reject those valid calls. Use the existingStringalias and clarify the parameter documentation.Validation: the same bytes examples run on the original implementation; mypy rejects two calls before and accepts them afterward, and Pyright passes. The existing suite passes 488 tests, with one optional-native-backend skip and 100% statement/branch coverage. The repository's 160 Ruff diagnostics and one source-mypy error for the absent optional backend are identical on clean upstream and this patch.
Prepared and verified with Codex (GPT-6.1-sol) assistance.