Skip to content

Conversation

TimD1
Copy link
Contributor

@TimD1 TimD1 commented Sep 10, 2025

This is a documentation-only PR to introduce documentation for the VariantRecordSample class.

@TimD1 TimD1 changed the title feat: added VariantRecordSample documentation feat(docs): added VariantRecordSample documentation Sep 10, 2025
@TimD1
Copy link
Contributor Author

TimD1 commented Sep 10, 2025

Any ideas why FreeBSD is now failing? Nothing should be changed except docstrings and the api.rst file...

@jmarshall
Copy link
Member

jmarshall commented Sep 10, 2025

Thanks for writing up some documentation.

For no particular reason, the FreeBSD CI workflow installs both build and test prerequisites up front, while others install test prerequisites later between the build and test steps. This means in the FreeBSD CI environment the build occurs with HTSlib htslib/*.h headers installed in /usr/local/include, whereas in the other CI environments samtools/htslib/bcftools are not installed until after the build has occurred.

The FreeBSD-built libcbcftools.cpython-311.so is failing due to a missing bcf_format_gt_v2(), which was introduced in HTSlib 1.22. The FreeBSD htslib package has (recently?) been updated to 1.22, while pysam on master still wraps 1.21.

So the bug here is that (at least on FreeBSD) building the Cython modules has preferred the HTSlib system headers rather than the ones within the pysam build tree. Which is annoying and hopefully easy enough to fix. Also something I didn't realise could be an issue here, so this is a useful heads-up…

@TimD1 TimD1 force-pushed the td_407_add-variant-record-sample-docs branch 3 times, most recently from c86f9b5 to 235524d Compare September 14, 2025 16:37
@jmarshall
Copy link
Member

-CC: "clang -I/usr/local/include"
+CC: "clang"

Yeah, that'd explain it…

This is fixed on master now, so if you rebase your documentation commits on that you can drop your FreeBSD CI patches from this PR.

FYI no need to use conventional commit message annotations in this repository. We'll remove them and put brief meaningful commit message prose instead on merging, so you'd be saving the maintainers work by not using them.

@TimD1 TimD1 changed the title feat(docs): added VariantRecordSample documentation Add VariantRecordSample documentation Sep 15, 2025
@TimD1 TimD1 force-pushed the td_407_add-variant-record-sample-docs branch from 235524d to d1c7d90 Compare September 15, 2025 14:46
@TimD1 TimD1 changed the title Add VariantRecordSample documentation Add VariantRecordSample documentation Sep 15, 2025
@TimD1
Copy link
Contributor Author

TimD1 commented Sep 15, 2025

Great, thanks for the quick fix! Rebased, should be ready to merge.

Regarding conventional commit messages, would you prefer I avoid them just in PR names, or all commits?

@@ -3460,7 +3485,7 @@ cdef class VariantRecordSample(object):

@property
def allele_indices(self):
"""allele indices for called genotype, if present. Otherwise None"""
"""Allele indices (e.g. `(0, 1)`) for the called genotype (if present), otherwise `None`."""
Copy link
Member

@jmarshall jmarshall Sep 18, 2025

Choose a reason for hiding this comment

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

This (and similarly elsewhere) needs to be `` … `` to get <code> rather than a default role which is just italicised. And None appears a lot so just use plain text for that.

Suggested change
"""Allele indices (e.g. `(0, 1)`) for the called genotype (if present), otherwise `None`."""
"""Allele indices (e.g. ``(0, 1)``) for the called genotype (if present), otherwise None."""

@jmarshall
Copy link
Member

jmarshall commented Sep 18, 2025

I've pushed the trivial parts of your api.rst changes (so that the autoclass:: pysam.VariantRecordSample addition will be clearer in the next commit), so you might want to rebase onto current master again please.

Re conventional commit styling on messages: ideally on neither PR titles (we don't use the GitHub setting that (ab)uses PR titles and first comments to override the author's commit messages on merge, so it's just noise in the PRs web page) nor commit messages (we don't find them useful in the commit history).

@TimD1 TimD1 force-pushed the td_407_add-variant-record-sample-docs branch from d1c7d90 to dc5ca68 Compare September 19, 2025 19:07
@TimD1 TimD1 force-pushed the td_407_add-variant-record-sample-docs branch from dc5ca68 to b401f11 Compare September 19, 2025 19:13
@TimD1
Copy link
Contributor Author

TimD1 commented Sep 19, 2025

I've rebased and made the requested modifications. I'll let you know once I've updated my next two PRs to follow your suggestions.

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.

how to set values in VariantRecordSamples and VariantRecordFormat ?? VariantRecordSample Documentation
2 participants