Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add ability to fetch number of sequences and I-th sequence from FAI index #377

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

jamorrison
Copy link
Contributor

faidx.c has two functions for retrieving the number of sequences in the FAI index (faidx_nseq) and the name of the I-th sequence (faidx_iseq). Previously these two functions weren't accessible through rust-htslib. I've added a function to access each of these into the faidx module.

This is my first PR for rust-htslib, so let me know if there are any things I can improve upon in my code!

@jamorrison jamorrison changed the title Add ability to fetch number of sequences and I-th sequence from FAI index feat: Add ability to fetch number of sequences and I-th sequence from FAI index Feb 23, 2023
@jamorrison
Copy link
Contributor Author

@johanneskoester Would mind reviewing my PR? Thank you!

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I would rename the functions a bit, see below.

src/faidx/mod.rs Outdated Show resolved Hide resolved
src/faidx/mod.rs Outdated Show resolved Hide resolved
jamorrison and others added 2 commits March 22, 2023 15:06
Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
@jamorrison
Copy link
Contributor Author

Thanks @johanneskoester for the comments. I committed your suggested name changes.

@brentp
Copy link
Contributor

brentp commented Mar 29, 2023

I need this functionality, anything holding up the PR?

@brainstorm
Copy link
Member

I need this functionality, anything holding up the PR?

Hi @brentp, code LGTM and CI passes (allowed the CI run yesterday). I'd guess that perhaps a matching test might be what's holding this from getting merged? Up to Johannes! ;)

@brentp
Copy link
Contributor

brentp commented Mar 29, 2023

Thanks @brainstorm .
@jamorrison , you can take my tests nearly verbatim from #381

@jamorrison
Copy link
Contributor Author

Thanks @brentp for the test code!

@johanneskoester I've added added tests for the two functions in the PR. Let me know if there's further work that needs to be done to wrap up this PR.

@johanneskoester
Copy link
Contributor

Awesome guys, thank you! In general: I often miss github notifications because I get so many. Just ping me via discord, twitter or email if I fail to respond in time.

@johanneskoester johanneskoester merged commit 6ecc4bd into rust-bio:master Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants