Ensure all sam_read1() and sam_itr_next() failures are reported#1379
Ensure all sam_read1() and sam_itr_next() failures are reported#1379daviesrob merged 2 commits intosamtools:developfrom
Conversation
Ensure that error return codes from all invocations of these functions are captured and result in a diagnostic message and failing exit status. For coverage, depth, fixmate, mpileup: the command already exited with failure but this adds an error message. For flagstat: convert a "Continue anyway" message into an error and failure. For bedcov, phase, targetcut: add the missing bam_[m]plp_auto() check and error message and failure. For tview: add the missing sam_itr_next() check, and exit immediately (as tview already does for other "can't happen" errors.)
daviesrob
left a comment
There was a problem hiding this comment.
Thanks for the updates, these are certainly long overdue.
Some of them seem to keep the "continue anyway" behaviour, albeit with returning an error code now. I think it might be time to make them bail out as soon as the error is found, as the output is unlikely to be useful anyway.
IMHO the defining quality of the “continue anyway” messages is that they are reassuring, which these aren't. However I hadn't tested For Thanks for the review. |
|
Thanks for the updates, will squash and merge. |
Ensure that error return codes from all invocations of these functions are captured and result in a diagnostic message and failing exit status. An audit of the 32 instances of
sam_read1()and the 10 instances ofsam_itr_next()in code contributing to thesamtoolsexecutable showed that the following were missing and are fixed by this PR:For coverage, depth, fixmate, mpileup: the command already exited with failure but this adds an error message. (For mpileup, this changes the exit status in this case from 255 (i.e., -1 🤮) to 1.)
For flagstat: convert a "Continue anyway" message into an error and failure.
For bedcov, phase, targetcut: add the missing
bam_[m]plp_auto()check and error message and failure.For tview: add the missing
sam_itr_next()check, and exit immediately (as tview already does for other "can't happen" errors.)Fixes #101. Fixes a number of instances of #51.