Skip to content

Conversation

@magicDGS
Copy link
Member

@magicDGS magicDGS commented Jul 4, 2018

No description provided.

@magicDGS
Copy link
Member Author

magicDGS commented Jul 4, 2018

Still requires some implementation of default behaviour, but depends on the discussion about API definitions.

@magicDGS magicDGS requested review from jacarey, lbergelson and tfenne July 4, 2018 13:17
* @author Daniel Gomez-Sanchez (magicDGS)
* @implSpec note that 0-length intervals are allowed in an implementation-dependent manner.
*/
public interface Locatable {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should extend Comparable, but I think that it will be better to have a LocatableComparator instead to allow different comprators to be use

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be comparable because there's no single consistent comparator, it depends on how you want to sort he chromosomes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't do.

* @see #getStart()
* @see #getEnd()
*/
default OptionalLong getLengthOnReference() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about not implementing this in the interface, but an utility class instead which throws if no end is provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Forget about this comment.

* @implSpec should return {@code false} if {@code referenceMatch(other) == false}.
* @see #withinDistanceOf(Locatable, int)
*/
default boolean overlaps(Locatable other) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can add also more methods to pad/merge/etc. to make the interface complete...

* with respect to a reference).
* </p>
*
* @author Daniel Gomez-Sanchez (magicDGS)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want author's tags in the new code? My feeling has been that they rapidly become out of date and are handled better by git history. Do others feel differently?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was the default in my IDE - will remove!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

* > getEnd()}).
* @see #getStart()
*/
OptionalLong getEnd();
Copy link
Member

Choose a reason for hiding this comment

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

Why make this optional? In a 1-based system a point just has end == start. Better to just require it I think and a point class can avoid storing it if they want. Adding an optional adds boxing overhead and complexity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Returned as long now.

@@ -0,0 +1,140 @@
package org.samtools.htsjdk.core;
Copy link
Member Author

Choose a reason for hiding this comment

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

How do you feel by making a package that is called org.samtools.htsjdk.core.api for the core interfaces and then a implementation-specific package? For example (just care about the api name, the rest are just first idea of how to name specific implementations):

  • Read interface in the api package, and SAMRecord implementation in org.samtools.htsjdk.core.sam
  • Variant interface in the api package, and VCFRecord implementation in org.samtools.htsjdk.core.vcf

@magicDGS magicDGS mentioned this pull request Jul 19, 2018
@magicDGS magicDGS force-pushed the dgs_locatable_interface branch from 6c79a97 to 4622ded Compare September 9, 2018 13:31
@magicDGS
Copy link
Member Author

magicDGS commented Sep 9, 2018

Rebased into the latest master (build system parts are removed).

@magicDGS magicDGS force-pushed the dgs_locatable_interface branch from 4622ded to cda6f0a Compare September 9, 2018 13:55
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