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.

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

[ADAM-582] Eliminate .get on option in FragmentCoverter. #1091

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@fnothaft
Member

fnothaft commented Jul 20, 2016

Resolves #582. In the FragmentConverter, we had code that called .get on an optional ReferenceRegion. If a NucleotideContigFragment didn't have a contig associated with it, then we would call .get on the empty option, which would throw an error. This commit changes that code to call map on the Option[ReferenceRegion], and then changes the usage of that method so that it is called in a flatMap. Additionally, I've added a test to cover this case.

[ADAM-582] Eliminate .get on option in FragmentCoverter.
Resolves #582. In the FragmentConverter, we had code that called .get on an
optional ReferenceRegion. If a NucleotideContigFragment didn't have a contig
associated with it, then we would call .get on the empty option, which would
throw an error. This commit changes that code to call map on the
Option[ReferenceRegion], and then changes the usage of that method so that it
is called in a flatMap. Additionally, I've added a test to cover this case.
@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jul 20, 2016

Member

+1

Member

heuermh commented Jul 20, 2016

+1

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins Jul 20, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1359/
Test PASSed.

AmplabJenkins commented Jul 20, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1359/
Test PASSed.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Jul 20, 2016

Member

Merged commit 7c2df60

Thank you, @fnothaft!

Member

heuermh commented Jul 20, 2016

Merged commit 7c2df60

Thank you, @fnothaft!

@heuermh heuermh closed this Jul 20, 2016

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