refactor: replace assert with raise in FastxZipped.__next__ - #307
Conversation
`FastxZipped.__next__` used `assert all_not_none(...)` in two places to check that no FASTX record is missing a name. Both are stripped under `python -O`, so in an optimized interpreter a nameless record would flow onward and fail later with a confusing error far from the cause. Replace both with an explicit guard that raises `ValueError`, matching the two neighbouring raises in the same method and the class docstring's promise that "an exception will be raised if any of the input files are malformed". The guard narrows exactly as the assert did, because a branch ending in `raise` terminates just as `assert` does, so no `cast()` is needed. Note this is defensive hardening, not a reachable bug: `FastxRecord.name` is typed `str | None`, but pysam's parser assigns at least the empty string to every record it reads, so only a directly-constructed `FastxRecord()` has a `None` name. The check cannot fire from file input. Extract the message into `_unnamed_record_message` and document that reasoning there, replacing the `# type narrowing` comments, which understated what the checks were doing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 participants
Note
Authorship: the content below was drafted by an AI coding agent and filed via gh under @msto's account. It reflects the agent's analysis, not a statement authored by @msto
Summary
FastxZipped.__next__usedassert all_not_none(...)in two places to check that no FASTX record is missing a name. Both are stripped underpython -O, so in an optimized interpreter a nameless record would flow onward and fail later, far from the cause.Both are now explicit guards that raise
ValueError, matching the two neighbouring raises in the same method and the class docstring's promise that "an exception will be raised if any of the input files are malformed or truncated".Narrowing is unchanged. A branch ending in
raiseterminates just asassertdoes, soall_not_none'sTypeGuardstill narrows on fall-through and nocast()is needed. I verified this holds in mypy, pyrefly, and basedpyright.This is hardening, not a reachable bug
Worth stating plainly so the change isn't over-read.
FastxRecord.nameis typedstr | None, but pysam's parser assigns at least the empty string to every record it reads:name>\nACGT''@\nACGT\n+\nIIII''> desc only\nACGT''(comment'desc only')FastxRecord()constructed directlyNoneOnly the last produces
None, andFastxZippednever constructs records — it only reads them. So the guard cannot fire from file input.That is also why there is no new test here: reaching the branch would mean replacing the private
_fastxhandles with fabricated records, which tests the mock rather than the behavior and would break under refactoring. Happy to add it if you'd rather have the line covered.Why
ValueErrorand notfail()This deliberately does not depend on #306.
ValueErroris what the two adjacent raises in this method already use for malformed input, and what the class docstring advertises;RequirementErrorwould introduce a second exception type into one method for no caller benefit.fail()would narrow identically if you'd prefer it — the choice is about the error contract, not the typing.Other changes
The message moves into
_unnamed_record_message, shared by both call sites, whose docstring records the pysam reasoning above. This replaces the# type narrowingcomments, which understated the checks — they were guarding a real (if unreachable) invariant, not just steering the type checker.poe check-allis green: lock, format, lint, mypy, 842 tests, doctests.🤖 Generated with Claude Code