fix: add null check to tidname on HeaderView#515
Conversation
dlejeune
left a comment
There was a problem hiding this comment.
Hey @minorllama thanks so much for this PR. Two things:
-
Can you remove the commented out code please
-
More a thought, but would it be worth changing the return type of
tid2nameto be anOption<&[u8]>instead of returning an empty string? Not necessarily something to change now since that breaks the API, but maybe something to consider.
|
@dlejeune Removed commented out old code with this commit . Yes, I agree it really should return an option as the getting back an empty string can be ambiguous, but it will break the api and will involve some refactoring. To log, this is where I see NULL being returned from the htslib. |
|
Thanks @minorllama. Can you correct the errors raised by the Formatting and Linting workflows? You just need to run |
$ cargo fmt ; RUSTFLAGS="-Dwarnings" cargo clippy --all-features --all-targets -- -Dclippy::all -Dunused_imports ; git add -u -v
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
add 'src/bam/mod.rs'and pushed. I was looking for a |
|
Thanks @minorllama. I see it's still failing on the linting but it's not related to your code so I'm happy with this this PR. Yeah, there's no contrib.md. It's on my TODO list to write one at some point, but if you want to submit one then please do. @johanneskoester I'm happy for this PR to be merged since the linting errors aren't related to the code changes. |
Coverage Report for CI Build 29440250837Coverage increased (+0.05%) to 82.129%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This fixes: #505 by adding a null check to
tid2name. also, added a test for the segfaulting example. Returning emptyCStron missing tid (when the C ffi return NULL) should also be documented.