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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Jul 1, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48a11dd
add documention for VariantContext.getStart() regarding telomeric events
SHuang-Broad May 17, 2019
f528993
reformating doc following comments
SHuang-Broad May 19, 2019
5f3229b
oops
SHuang-Broad May 19, 2019
b2961e5
2nd round
SHuang-Broad May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/main/java/htsjdk/variant/variantcontext/VariantContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,7 +1664,15 @@ public String getContig() {

/**
* @return 1-based inclusive start position of the Variant
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are here, I would reformat the javadoc like this:

/**
 * Summary sentence.
 *<p>
*    explanation of what it does without loosing time in details of particular edge cases.
 *</p>
* <p>
*   edge case-1
*</p>
* <p>
*   edge case-2
* </p>
*
* @return the text above should had made clear wha is returned... here you report on the possible range of values very briefly.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

For example in this case something like this:

/**
  * Returns the (start) position of the variant.
  * <p>
  *   Main blah blah ... 1-based  ... blah blah.
  *</p>
  *<p>
  * For telomeres  blah blah can be 0  or N+1 blah blah
  *</p>
  * @return 0 or greater, never a negative number.
 */

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I'm just putting emphasis in the summary sentence (first one finished with .) and the statement in the [at]return tag, the details in the section in between is up to you (that is way I add those "blah" "blah").

That said I think you should be more concise... The programmer don't want to expend to much time to understand what may happen in 0.1% percent of cases. For example I would not include details on what is said or depicted in the spec, just simply refer to it for further reading/details.

* INDEL events usually start on the first unaltered reference base before the INDEL
* INDEL events usually start on the first unaltered reference base before the INDEL.
*
* Note also that for the VCF spec allows 0 and N + 1 for POS field, where N is the length of the chromosome,
* for telomeric event.
* The given example is in section 5.4.5 (see example event illustrated in Figure 6 and VCF records below the figure).
* Another possible scenario is when a telomere on the p-arm of a chromosome is deleted.
* In these cases, the "0" value returned should be interpreted as telomere, and does not violate the above "1-based" comment.
* It's the responsibility of code generating such variants to make sure {@code start} is populated correctly.
* And code consuming the returned {@code start} should be prepared for such out-of-the-ordinary values.
*
* <strong>Warning:</strong> be aware that the start position of the VariantContext is defined in terms of the start position specified in the
* underlying vcf file, VariantContexts representing the same biological event may have different start positions depending on the
Expand Down