/htsjdk

Added getDescription() in VCFFilterHeaderLine #726

Merged
merged 2 commits into from Nov 22, 2016

Conversation

Reviewers

@yfarjoun yfarjoun

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@lindenb @coveralls @yfarjoun @lbergelson
@lindenb
Contributor

lindenb commented Oct 18, 2016

Description

Using a VCFFilterHeaderLine as a java-bean, I was surprised to see that there is no (?) public/easy way to get the "Description" of a VCFFilterHeaderLine . This PR adds the method getDescription() ; It returns the ID if the description is empty.
I've also added some final keywords and the serialVersionUID

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
  • Extended the README / documentation, if necessary
  • Is not backward compatible (breaks binary or source compatibility)
@lindenb lindenb Added getDescription() in VCFFilterHeaderLine

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.01%) to 69.052%
Details
c132c93
@coveralls

coveralls commented Oct 18, 2016

Coverage Status

Coverage increased (+0.01%) to 69.052% when pulling c132c93 on lindenb:filterdesc into 88b6719 on samtools:master.
src/main/java/htsjdk/variant/vcf/VCFFilterHeaderLine.java
+ */
+ public String getDescription() {
+ final String desc = getGenericFieldValue("Description");
+ return ( StringUtil.isBlank(desc) ? getID() : desc );
@yfarjoun

yfarjoun Oct 27, 2016

Contributor

I think that this logic should not be in htsjdk. getDescription() should return null or blank if there's no decription. the user can choose what to do with that information.

@lindenb lindenb changed according to #726 (review)

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.7%) to 69.742%
Details
8dccd62
@yfarjoun
Contributor

yfarjoun commented Oct 27, 2016

This looks fine to me. I'll leave it for a few more days to see if there are other comments and merge it otherwise.
@coveralls

coveralls commented Oct 27, 2016

Coverage Status

Coverage increased (+0.7%) to 69.742% when pulling 8dccd62 on lindenb:filterdesc into 88b6719 on samtools:master.

@lbergelson lbergelson merged commit 4558e37 into samtools:master Nov 22, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 69.742%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment