check for negative length in CigarElement ctor #839

SHuang-Broad · Apr 3, 2017

Description

Class CigarElement currently doesn't check for the length provided in its ctor. This PR adds that check.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
[NA] Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

codecov-io · Apr 3, 2017

Codecov Report

Merging #839 into master will decrease coverage by 0.005%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master      #839       +/-   ##
===============================================
- Coverage     64.863%   64.857%   -0.005%     
+ Complexity      7197      7195        -2     
===============================================
  Files            527       527               
  Lines          31781     31782        +1     
  Branches        5424      5425        +1     
===============================================
- Hits           20614     20613        -1     
+ Misses          9021      9020        -1     
- Partials        2146      2149        +3

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/CigarElement.java	`58.824% <100%> (+2.574%)`	`6 <0> (+1)`	⬆️
...dk/samtools/seekablestream/SeekableHTTPStream.java	`54.545% <0%> (-1.515%)`	`9% <0%> (-1%)`
...main/java/htsjdk/samtools/SAMRecordSetBuilder.java	`88.306% <0%> (-0.806%)`	`62% <0%> (-2%)`
...va/htsjdk/samtools/util/AsyncBufferedIterator.java	`69.565% <0%> (+1.087%)`	`17% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d343e7...939e128. Read the comment docs.

lbergelson · Apr 4, 2017

lbergelson reviewed Apr 4, 2017

View changes

One minor comment to include a bit more information in the error message. 👍 After that.

Can you include the element type in the output message as well?

here you go. thanks @lbergelson

SHuang-Broad · Apr 4, 2017

Made the change, back to you @lbergelson

SHuang-Broad · Apr 4, 2017

Sorry for forgetting to mention this, @lbergelson . I don't have write access to this repo. So if you approve the changes and feel like it, please help me squash and merge.
Thanks!

yfarjoun · Apr 16, 2017

$@#{review_dismissed_event.actor.login}$ yfarjoun dismissed lbergelson’s review Apr 16, 2017

requested changes were made.

yfarjoun · Apr 16, 2017

👍 thanks @SHuang-Broad

SHuang-Broad changed the title from Sh cigar ele arg check to check for negative length in CigarElement actor Apr 3, 2017

SHuang-Broad changed the title from check for negative length in CigarElement actor to check for negative length in CigarElement ctor Apr 3, 2017

SHuang-Broad was assigned by lbergelson Apr 4, 2017

		SHuang-Broad	`added check for negative length in CigarElement ctor`			`fc58f61`
		SHuang-Broad	`added test`		All checks have passed 4 successful checks codecov/changes — No unexpected coverage changes found. Details codecov/patch — 100% of diff hit (target 64.863%) Details codecov/project — 64.876% (+0.014%) compared to 9d343e7 Details continuous-integration/travis-ci/pr — The Travis CI build passed Details	`5fef0ba`

@@ -36,6 +36,7 @@
          private final CigarOperator operator;
          public CigarElement(final int length, final CigarOperator operator) {
+    +        if (length < 0) throw new IllegalArgumentException("Cigar element being constructed with negative length: " + length);

		SHuang-Broad	`improved error message by including operation type`		All checks have passed 4 successful checks codecov/changes — No unexpected coverage changes found. Details codecov/patch — 100% of diff hit (target 64.863%) Details codecov/project — 64.867% (+0.004%) compared to 9d343e7 Details continuous-integration/travis-ci/pr — The Travis CI build passed Details	`6553d56`
		SHuang-Broad	`sorry for the typo`		Some checks were not successful 1 failing and 3 successful checks codecov/changes — 2 files have unexpected coverage changes not visible in diff. Details codecov/patch — 100% of diff hit (target 64.863%) Details codecov/project — Absolute coverage decreased by -0.005% but relative coverage increased by +35.137% compared to 9d343e7 Details continuous-integration/travis-ci/pr — The Travis CI build passed Details	`939e128`

samtools/htsjdk

check for negative length in CigarElement ctor #839

SHuang-Broad commented Apr 3, 2017

SHuang-Broad added some commits Apr 3, 2017

SHuang-Broad changed the title from Sh cigar ele arg check to check for negative length in CigarElement actor Apr 3, 2017

SHuang-Broad changed the title from check for negative length in CigarElement actor to check for negative length in CigarElement ctor Apr 3, 2017

codecov-io commented Apr 3, 2017 • edited

lbergelson reviewed Apr 4, 2017

View changes

SHuang-Broad was assigned by lbergelson Apr 4, 2017

SHuang-Broad added some commits Apr 4, 2017

SHuang-Broad commented Apr 4, 2017

SHuang-Broad commented Apr 4, 2017

$@#{review_dismissed_event.actor.login}$ yfarjoun dismissed lbergelson’s review Apr 16, 2017

yfarjoun commented Apr 16, 2017

yfarjoun merged commit `8c090ce` into samtools:master Apr 16, 2017
3 of 4 checks passed

3 of 4 checks passed

samtools/htsjdk

Join GitHub today

check for negative length in CigarElement ctor #839

Conversation

SHuang-Broad commented Apr 3, 2017

Description

Checklist

SHuang-Broad added some commits Apr 3, 2017

All checks have passed

SHuang-Broad changed the title from Sh cigar ele arg check to check for negative length in CigarElement actor Apr 3, 2017

SHuang-Broad changed the title from check for negative length in CigarElement actor to check for negative length in CigarElement ctor Apr 3, 2017

codecov-io commented Apr 3, 2017 • edited

Codecov Report

lbergelson reviewed Apr 4, 2017 View changes

lbergelson Apr 4, 2017

SHuang-Broad Apr 4, 2017

SHuang-Broad was assigned by lbergelson Apr 4, 2017

SHuang-Broad added some commits Apr 4, 2017

All checks have passed

Some checks were not successful

SHuang-Broad commented Apr 4, 2017

SHuang-Broad commented Apr 4, 2017

yfarjoun dismissed lbergelson’s review Apr 16, 2017

yfarjoun commented Apr 16, 2017

Hide details View details yfarjoun merged commit 8c090ce into samtools:master Apr 16, 2017 3 of 4 checks passed

3 of 4 checks passed

lbergelson reviewed Apr 4, 2017

View changes

$@#{review_dismissed_event.actor.login}$ yfarjoun dismissed lbergelson’s review Apr 16, 2017

yfarjoun merged commit `8c090ce` into samtools:master Apr 16, 2017
3 of 4 checks passed