samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

add documention for VariantContext.getStart() regarding telomeric events #1369

Closed SHuang-Broad closed 5 years ago

SHuang-Broad commented 5 years ago

Description

The VCF spec allows POS column to take value 0 (or chromosome length + 1), when the event is at a telomere.

Currently the documentation for public int VariantContext.getStart() claims it returns a 1-based value, where it seems that 0 is an invalid value.

This PR intends to clarify that, by adding documentation.

Checklist

(Documentation change, most of the following does not apply)

codecov-io commented 5 years ago

Codecov Report

Merging #1369 into master will decrease coverage by 0.009%. The diff coverage is n/a.

@@              Coverage Diff               @@
##             master     #1369       +/-   ##
==============================================
- Coverage     67.85%   67.841%   -0.009%     
+ Complexity     8283      8282        -1     
==============================================
  Files           564       564               
  Lines         33695     33695               
  Branches       5650      5650               
==============================================
- Hits          22862     22859        -3     
- Misses         8653      8655        +2     
- Partials       2180      2181        +1
Impacted Files Coverage Δ Complexity Δ
.../htsjdk/variant/variantcontext/VariantContext.java 77.714% <ø> (ø) 246 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/BAMFileReader.java 67.847% <0%> (-0.817%) 51% <0%> (-1%)
SHuang-Broad commented 5 years ago

@vruano Thanks for taking a look! Please take a look at the re-formatted doc.

vruano commented 5 years ago

In my view is just too much text; think about the 99.5% of people that don't care about telomeres. but I rather you get some other opinion as perhaps I'm just too pedantic.

About the summary and [at]return annotation. I mean something like this:

/**
 * Returns the position for this variant.
 *
 * @return 0 or greater.
 */

The summary sentence is "what it does" very briefly whareas the [at]return is just give info about the possible range of values without semantics (this goes into the summary and the rest of the javadoc. For example if the return was Object the often is either @return never {@code null} or @return might be {@code null}. Sometimes if the method is know to return this (e.g. StringBuilder append methods) then I also say that @return this builder (in this case it is imply that it's never null.

vruano commented 5 years ago

I would add <strong>warning</strong> people that work to this level with VCF data are supposed to be aware of this.

SHuang-Broad commented 5 years ago

I agree with you that the summary was too long. So I shrunk it further and added a warning to the new note about telomere.

vruano commented 5 years ago

Sorry perhaps I went to far giving examples ... can borrow mine but you can/should use your own words.