/htsjdk

IndexingVariantContextWriter cleanup #706

Merged
merged 1 commit into from Sep 20, 2016

Conversation

Reviewers

@cmnbroad cmnbroad

Assignees

@cmnbroad cmnbroad

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@magicDGS @coveralls @cmnbroad
@magicDGS
Contributor

magicDGS commented Sep 13, 2016 • edited

Description

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)
@coveralls

coveralls commented Sep 13, 2016

Coverage Status

Coverage remained the same at 68.921% when pulling 9aa92d1 on magicDGS:dgs_IndexingVariantContextWriter_cleanup into c3d5a88 on samtools:master.

cmnbroad was assigned by droazen Sep 13, 2016

...a/htsjdk/variant/variantcontext/writer/IndexingVariantContextWriter.java
@@ -140,7 +141,7 @@ public void close() {
// close the index stream (keep it separate to help debugging efforts)
if (indexer != null) {
if (indexer instanceof TribbleIndexCreator) {
- setIndexSequenceDictionary((TribbleIndexCreator)indexer, refDict);
+ ((TribbleIndexCreator)indexer).setIndexSequenceDictionary(refDict);
@cmnbroad

cmnbroad Sep 15, 2016 • edited

Contributor

I think if we're going to move setIndexSequenceDictionary so we can use it for other index types, we should just add it to the base interface (IndexCreator) with a default no-op implementation, and have TribbleIndexCreator override it. Then we can get rid of the instanceOf and downcast operators here, and other index types can use it if they choose to, or just ignore it.

@magicDGS

magicDGS Sep 16, 2016

Contributor

Done.

src/main/java/htsjdk/tribble/writers/PositionalOutputStream.java
+* THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+*/
+
+package htsjdk.tribble.writers;
@cmnbroad

cmnbroad Sep 15, 2016

Contributor

There isn't anything tribble-specific about this, so I wouldn't extend the namespace for this one file. I would just put it in the htsjdk.util package where the other stream wrappers live.

@magicDGS

magicDGS Sep 16, 2016

Contributor

I will extend the namespace later, while addressing #701. But I will change it anyway.

@magicDGS

magicDGS Sep 16, 2016

Contributor

Done

src/main/java/htsjdk/tribble/writers/PositionalOutputStream.java
+ * Wraps output stream in a manner which keeps track of the position within the file and allowing writes
+ * at arbitrary points
+ */
+public final class PositionalOutputStream extends OutputStream implements LocationAware
@cmnbroad

cmnbroad Sep 15, 2016

Contributor

Add tests since we're making this public.

@magicDGS

magicDGS Sep 16, 2016

Contributor

Added a very simple test to check that the position is the expected one.

@magicDGS
Contributor

magicDGS commented Sep 16, 2016

Addressed comments @cmnbroad -- back to you! Thanks for reviewing!
@coveralls

coveralls commented Sep 16, 2016

Coverage Status

Coverage increased (+0.06%) to 68.985% when pulling eb10ee4 on magicDGS:dgs_IndexingVariantContextWriter_cleanup into c3d5a88 on samtools:master.
...a/htsjdk/variant/variantcontext/writer/IndexingVariantContextWriter.java
@@ -26,14 +26,14 @@
package htsjdk.variant.variantcontext.writer;
import htsjdk.samtools.SAMSequenceDictionary;
-import htsjdk.samtools.SAMSequenceRecord;
import htsjdk.samtools.util.LocationAware;
import htsjdk.samtools.util.RuntimeIOException;
import htsjdk.tribble.index.DynamicIndexCreator;
import htsjdk.tribble.index.Index;
import htsjdk.tribble.index.IndexCreator;
import htsjdk.tribble.index.IndexFactory;
import htsjdk.tribble.index.TribbleIndexCreator;
@cmnbroad

cmnbroad Sep 16, 2016

Contributor

Can you remove this import - its now orphaned.

@magicDGS

magicDGS Sep 17, 2016

Contributor

Done.

src/test/java/htsjdk/samtools/util/PositionalOutputStreamTest.java
+ // check that write a byte array adds its length
+ final byte[] bytes = new byte[]{1, 3, 5, 7};
+ wrapped.write(bytes);
+ Assert.assertEquals(wrapped.getPosition(), bytes.length + 1);
@cmnbroad

cmnbroad Sep 16, 2016

Contributor

Thanks for adding this test. One minor request for clarity - can you keep the running count in a variable so that each assert doesn't need to recalculate the running total.

@magicDGS

magicDGS Sep 17, 2016

Contributor

Done.

@cmnbroad
Contributor

cmnbroad commented Sep 16, 2016

@magicDGS A couple of minor cleanup requests, then squash and rebase. Thx.
@coveralls

coveralls commented Sep 17, 2016

Coverage Status

Coverage increased (+0.08%) to 69.001% when pulling 6eeddbe on magicDGS:dgs_IndexingVariantContextWriter_cleanup into c3d5a88 on samtools:master.
@magicDGS @magicDGS magicDGS IndexingVariantContextWriter cleanup 
  * extract PositionalOutputStream
  * extract setIndexSequenceDictionary to IndexCreator with no-op default and TribbleIndexCreator implementation

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage remained the same at 68.995%
Details
c4dd9c0
@magicDGS
Contributor

magicDGS commented Sep 17, 2016

Squashed and rebased. Ready to go, @cmnbroad. Thank you very much!
@coveralls

coveralls commented Sep 17, 2016

Coverage Status

Coverage remained the same at 68.995% when pulling c4dd9c0 on magicDGS:dgs_IndexingVariantContextWriter_cleanup into fbba536 on samtools:master.
@coveralls

coveralls commented Sep 17, 2016

Coverage Status

Coverage remained the same at 68.995% when pulling c4dd9c0 on magicDGS:dgs_IndexingVariantContextWriter_cleanup into fbba536 on samtools:master.

@cmnbroad cmnbroad merged commit 88b6719 into samtools:master Sep 20, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 68.995%
Details

cmnbroad referenced this pull request Sep 20, 2016

Closed

Request: public access to inner class PositionalOutputStream #702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment