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

Don't not add a period to output basenames #1430

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

tlangs
Copy link
Contributor

@tlangs tlangs commented Nov 19, 2019

Description

A change was introduced last year to CheckFingerprint which made it only add a period to the end of the OUTPUT argument if it did not already end with a period. This causes errors in our pipeline when sample aliases end in a period, because we derive our cromwell outputs based on the sample alias concatenated with "fingerprinting_summary_metrics" and "fingerprinting_detail_metrics". If sample_alias is MyCoolSampleG.T., the output files should be MyCoolSampleG.T..fingerprinting_summary_metrics and MyCoolSampleG.T..fingerprinting_detail_metrics


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@lbergelson
Copy link
Member

@tlangs It's pretty confusing to have a sample name that ends in a period. Can we prevent that?

@tlangs
Copy link
Contributor Author

tlangs commented Nov 19, 2019

The tool should respect what it's given, no matter if it looks ugly. Changing the output name is not documented and results in a lot of confusion when the tool spits out files with different names from what was specified and expected. I don't think its up to us dictate how people name their files.

@lbergelson
Copy link
Member

I agree we should fix the tool, but it would also be nice if we did dictate what people put in their sample names so they didn't have dumb things like a period as the last character.

Copy link
Contributor

@tbl3rd tbl3rd left a comment

Choose a reason for hiding this comment

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

"Don't not"?

@tlangs tlangs merged commit 4d64eb4 into master Nov 20, 2019
@tlangs tlangs deleted the tl_dont_add_period_to_output_name branch November 20, 2019 20:57
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