Skip to content

Add missing NULL guard in PileupColumn.get_query_qualities#1380

Draft
nh13 wants to merge 1 commit into
pysam-developers:masterfrom
nh13:nh13/fix-pileup-qualities-null-guard
Draft

Add missing NULL guard in PileupColumn.get_query_qualities#1380
nh13 wants to merge 1 commit into
pysam-developers:masterfrom
nh13:nh13/fix-pileup-qualities-null-guard

Conversation

@nh13

@nh13 nh13 commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add the missing plp == NULL guard to PileupColumn.get_query_qualities() to match all 6 sibling methods (pileups, get_num_aligned, get_query_sequences, get_mapping_qualities, get_query_positions, get_query_names)
  • Without this guard, calling get_query_qualities() after the pileup iterator is exhausted causes a segfault instead of raising a clean ValueError

Test plan

  • Run REF_PATH=: pytest tests/AlignmentFilePileup_test.py -v

AI disclosure: This PR was AI-assisted using Claude Code. The issue was identified via AI-guided code review, and the implementation was drafted by AI with human review and approval.

@nh13 nh13 force-pushed the nh13/fix-pileup-qualities-null-guard branch from 0d22c0d to 9c654ed Compare March 2, 2026 05:12
@jmarshall

Copy link
Copy Markdown
Member
  1. For future reference, soon after a release would be a better time to swamp the maintainers with a deluge of small pull requests.
  2. If AI tools were used in the creation of a pull request, please disclose this and describe what aspects of the PR contain AI-derived contributions.
  3. Please do not use conventional commit formatting in your commit messages (or PR titles) in this repository.

@bioinformed

Copy link
Copy Markdown
Member

@nh13 : Thanks for putting in the time to improve pysam!

  1. For future reference, soon after a release would be a better time to swamp the maintainers with a deluge of small pull requests.

@jmarshall: Hopefully this is meant tongue-in-cheek because maintainers have no obligation to merge late-breaking pull requests if a release is impending. It also seems unwise to complain about small, targeted PRs. There are definitely better and worse times for contributions in terms of release latency, but I hope that useful contributions are always welcome.

I know this is easy for me to say, but I'll set aside some time in the next week to help review these PRs to hopefully save @jmarshall some time (who is an absolute hero for his work keeping pysam afloat).

@nh13 nh13 marked this pull request as draft March 3, 2026 06:21
@nh13 nh13 changed the title fix: add missing NULL guard in PileupColumn.get_query_qualities Add missing NULL guard in PileupColumn.get_query_qualities Mar 3, 2026
@nh13 nh13 force-pushed the nh13/fix-pileup-qualities-null-guard branch from 9c654ed to b124bcb Compare March 3, 2026 06:24
@nh13

nh13 commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

@jmarshall, apologies for the deluge and the poor timing. I should have reached out first to ask whether these were welcome and when. I've converted all 13 PRs to draft so they're out of your way for the release.

On your points:

  1. Timing: Totally fair. I'll coordinate better going forward.
  2. AI disclosure: Done. I've added a disclosure to each PR description. These were AI-assisted using Claude Code: the issues were identified via AI-guided code review, and the implementations were drafted by AI with my review and approval.
  3. Conventional commits: Done. I've rewritten all commit messages and PR titles to match the project's style.

Happy to close any or all of these if they're not useful, or leave them as drafts until you're open to reviewing them. Whatever works best for you.

@bioinformed, thanks for the kind words and the offer to help review.

@jmarshall

Copy link
Copy Markdown
Member

There are test cases for these accessors — see testIteratorOutOfScope() in tests/AlignmentFilePileup_test.py. On older Pythons, they all work as expected with or without this PR — in the case of get_query_qualities() a ValueError is raised in these circumstances a few lines lower down. Nonetheless it would be good to apply your PR to regularise the validity checks.

However none of this works in Python 3.11, which is #1151. Rather to my surprise, it turns out these test cases do work again — and don't crash — in Python 3.12 and later! Nonetheless I'd like to use this PR as an additional excuse to get to the bottom of that Python 3.11 problem and hopefully merge a fix for that at the same time.

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.

3 participants