/htsjdk

Methods for default sequence dictionary name #774

Merged
merged 3 commits into from Dec 28, 2016

Conversation

Reviewers

@yfarjoun yfarjoun

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@magicDGS @coveralls @yfarjoun
@magicDGS
Contributor

magicDGS commented Dec 14, 2016 • edited

Description

Changes for broadinstitute/picard#712 and broadinstitute/gatk#2243

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)

magicDGS added some commits Dec 14, 2016

@magicDGS magicDGS added new methdos to ReferenceSequenceFileFactory 07f8467
@magicDGS magicDGS using the methods to simplify code

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.004%) to 70.552%
Details
56f6243
@magicDGS
Contributor

magicDGS commented Dec 14, 2016

This is for two PRs under review in Picard and GATK. @yfarjoun and @cmnbroad, you are reviewing them...could you have a look?

magicDGS referenced this pull request in broadinstitute/picard Dec 14, 2016

Merged

Default output for CreateSequenceDictionary #712

3 of 5 tasks complete
@coveralls

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.004%) to 70.552% when pulling 56f6243 on magicDGS:dgs_ReferenceSequenceFileFactory_default_dict into 2386a6e on samtools:master.

magicDGS referenced this pull request in broadinstitute/picard Dec 14, 2016

Closed

CreateSequenceDictionary should use default HTSJDK method #714

src/main/java/htsjdk/samtools/reference/ReferenceSequenceFileFactory.java
+ *
+ * @throws IllegalArgumentException if the file is not a supported reference file.
+ */
+ public static String getFastaExtension(final Path path) {
@yfarjoun

yfarjoun Dec 16, 2016

Contributor

Path is supposed to allow URLs right? signed URLs have keys after the name of the file....so something like

http://my.web.site/file.fasta?key=abcd12345

should we start thinking about these kinds of URLs, or is it hopeless? we have similar problems with bam/vcfs and their index files...

@magicDGS

magicDGS Dec 27, 2016

Contributor

I don't know about this, maybe it is better to discuss it in #724...

@yfarjoun

yfarjoun requested changes Dec 25, 2016
View changes

thanks for this. looks good overall. a few requests...

...st/java/htsjdk/samtools/reference/ReferenceSequenceFileFactoryTests.java
+ public void testGetDefaultDictionaryForReferenceSequence(final String fastaFile, final String expectedDict) throws Exception {
+ Assert.assertEquals(ReferenceSequenceFileFactory.getDefaultDictionaryForReferenceSequence(new File(fastaFile)), new File(expectedDict));
+ }
+
@yfarjoun

yfarjoun Dec 25, 2016

Contributor

extra newline

@magicDGS

magicDGS Dec 27, 2016

Contributor

Removed

src/main/java/htsjdk/samtools/reference/ReferenceSequenceFileFactory.java
+ return new IndexedFastaSequenceFile(path);
+ }
+ catch (final FileNotFoundException e) {
+ throw new IllegalStateException("Should never happen, because existence of files has been checked.", e);
}
}
@yfarjoun

yfarjoun Dec 25, 2016

Contributor

use } else { on same line

@magicDGS

magicDGS Dec 27, 2016

Contributor

That was out of the PR, but fixed.

@magicDGS magicDGS address comments

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.002%) to 70.545%
Details
3f30e34
@magicDGS
Contributor

magicDGS commented Dec 27, 2016

Adreessed requests. Back to you @yfarjoun.
@coveralls

coveralls commented Dec 27, 2016

Coverage Status

Coverage decreased (-0.002%) to 70.545% when pulling 3f30e34 on magicDGS:dgs_ReferenceSequenceFileFactory_default_dict into 2386a6e on samtools:master.
@yfarjoun
Contributor

yfarjoun commented Dec 28, 2016

thanks!
@yfarjoun
Contributor

yfarjoun commented Dec 28, 2016

👍

@yfarjoun yfarjoun merged commit 0e91c4b into samtools:master Dec 28, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 70.545%
Details

magicDGS deleted the magicDGS:dgs_ReferenceSequenceFileFactory_default_dict branch Jan 9, 2017

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