Skip to content

Changed SAMRecord.toString() to emit the SAM format string with all fields#1762

Open
tfenne wants to merge 1 commit intomasterfrom
tf_getter_samrecord_tostring
Open

Changed SAMRecord.toString() to emit the SAM format string with all fields#1762
tfenne wants to merge 1 commit intomasterfrom
tf_getter_samrecord_tostring

Conversation

@tfenne
Copy link
Copy Markdown
Member

@tfenne tfenne commented Mar 30, 2026

Description

SAMRecord has historically output an abbreviated format in toString(). I think more often than not this is confusing to users, since a SAMRecord has a well defined String format. Moreover the toString() method is used extensively by TestNG to display when two SAMRecord instances are not equal - and the vast majority of the time the toString() output did not contain the differences.

Things to think about before submitting:

  • Make sure your changes compile and new tests pass locally.
  • Add new tests or update existing ones:
    • A bug fix should include a test that previously would have failed and passes now.
    • New features should come with new tests that exercise and validate the new functionality.
  • Extended the README / documentation, if necessary
  • Check your code style.
  • Write a clear commit title and message
    • The commit message should describe what changed and is targeted at htsjdk developers
    • Breaking changes should be mentioned in the commit message.

@tfenne tfenne requested a review from yfarjoun March 30, 2026 19:59
Copy link
Copy Markdown
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

  1. can you verify there's no newline, and if there is, strip it (he says knowingly)
  2. are there tools (picard, igv, gatk, fgbio, others) that rely on the toString returned format (I know I know), but it is a public so a breaking change...
  3. Should you add at least one regression test?
  4. toString was previously lock free, but looking at getSAMString we have some synchronized methods...
  5. the previous method was lighter-weight, is this a concern?
  6. are there tests that (shudders) compared outputs using toString?
  7. is it time to remove the format deprecated method?

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.

2 participants