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

GenotypeAllele encoding and documentation for pushing to Record #285

Merged
merged 6 commits into from
Dec 11, 2020
Merged

GenotypeAllele encoding and documentation for pushing to Record #285

merged 6 commits into from
Dec 11, 2020

Conversation

malthesr
Copy link
Contributor

Addresses the remaining points in #224:

  • Adds From<i32> for GenotypeAllele, copying the code from the old from_encoded method. Also included a bit of testing for this in the tests for the previous From<GenotypeAllele> for i32, ensuring that converting back and forth does nothing.
  • Replaces the only internal usage of from_encoded, deprecating the method from 0.36.0 onwards.
  • Adds module level documentation for setting up a minimal VCF writer with header and a record with chrom, pos, and genotype information.
  • Adds illustrative examples/doctests for three write methods for Record: set_rid, push_genotypes, and push_format_float.

The doc tests all require quite a bit of setup to compile. I've hidden this from the doc, with a note about the presuppositions and a reference to the module level docs, which gives a full example. I'm not sure this is the best way to do it, but including all the setup code seems to me to divert attention from the main point.

Moreover, the setup requires a lot of code duplication, which is not nice, but I couldn't find a way to centralise the setup so long as #[cfg(doctest)] doesn't work for this case (see Rust issue #67295).

Very happy to hear any other takes on how to organise this.

@coveralls
Copy link

coveralls commented Dec 10, 2020

Pull Request Test Coverage Report for Build 415805844

  • 17 of 18 (94.44%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 93.817%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/bcf/record.rs 8 9 88.89%
Files with Coverage Reduction New Missed Lines %
src/bcf/record.rs 6 74.4%
Totals Coverage Status
Change from base Build 379040825: -0.05%
Covered Lines: 10834
Relevant Lines: 11548

💛 - Coveralls

Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks! As a first quick pass, I'd fix the clippy variable does not need to be mutable warnings, there are several that seem easy to fix?

@malthesr
Copy link
Contributor Author

I saw those, but they were from before this pull request, so I figured maybe they served some point and left them. Anyway, that should be fixed now. There's still a couple of clippy warnings relating to code I'm not familiar with, though.

Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

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

Thanks @malthesr, I didn't realize the clippy warnings were there before, thanks for fixing!

I don't have strong opinions about your choices on the docs side of things, I reckon I'll defer the verdict to the other two reviewers @tedil and @dlaehnemann ;)

Copy link
Member

@dlaehnemann dlaehnemann left a comment

Choose a reason for hiding this comment

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

I'll just add my two cents regarding the doctests: I really like this split of hiding complexity that is not relevant to the functionality that the doctest is actually testing and showing, but pointing to the module docs for the necessary setup. This sounds like the solution we've been looking for all along! :D

One tiny thing: You included some relative links within the docs. Is there a way to link to the doctest in the module docs, directly. That way, the hidden complexity would simply be one click away. But never mind if this is not quickly done.

@malthesr
Copy link
Contributor Author

Good idea, @dlaehnemann, and thank you for the reviews from all three of you. I've added direct links from the three doc tests that mention the module level docs. (I also fixed the monospace font on two other links I'd missed last time).

@dlaehnemann dlaehnemann merged commit 490e367 into rust-bio:master Dec 11, 2020
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.

5 participants