/htsjdk

Fixed naming inconsistency in SamLocusIterator #715

Merged
merged 1 commit into from Jan 4, 2017

Conversation

Reviewers

@lbergelson lbergelson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@superbobry @coveralls @yfarjoun @lbergelson
@superbobry
Contributor

superbobry commented Sep 28, 2016

Description

The method named getRecordAndPositions returned a list of RecordAndOffset objects which was a bit confusing.

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 28, 2016

Coverage Status

Coverage decreased (-0.002%) to 69.038% when pulling 1ebc898 on superbobry:get-record-and-positions into 88b6719 on samtools:master.
@yfarjoun
Contributor

yfarjoun commented Nov 19, 2016

There's a large PR that is changing similar code (#637). Could you rebase after they merge?
@superbobry
Contributor

superbobry commented Nov 19, 2016

I think you can now merge-rebase via GitHub UI?
@yfarjoun
Contributor

yfarjoun commented Nov 20, 2016

not if there are conflicts, and there will be....that's why I want the other PR to go in first.
@superbobry
Contributor

superbobry commented Nov 20, 2016

Okay, just lmk when the other PR gets merged.
src/main/java/htsjdk/samtools/util/SamLocusIterator.java
@@ -106,7 +106,8 @@ public void addInserted(final SAMRecord read, int firstPosition) {
/** @return 1-based reference position */
public int getPosition() { return position; }
- public List<RecordAndOffset> getRecordAndPositions() { return Collections.unmodifiableList(recordAndOffsets); }
+ public List<RecordAndOffset> getRecordAndOffsets() { return Collections.unmodifiableList(recordAndOffsets); }
+ @Deprecated public List<RecordAndOffset> getRecordAndPositions() { return getRecordAndOffsets(); }
@lbergelson

lbergelson Nov 22, 2016

Contributor

Could you add a javadoc line linking to the replacement function. I know it's pretty obvious from the code but its good to make deprecations as fool proof as possible.

@yfarjoun
Contributor

yfarjoun commented Nov 23, 2016

@lbergelson please be advised that I asked that this PR wait for another larger one that will be in conflict with this one...
@yfarjoun
Contributor

yfarjoun commented Dec 6, 2016

This can now be rebased over the latest master.
@lbergelson @superbobry lbergelson updating the gradle version from 2.13 to 3.2.1 (#727) 
updating the gradle version from 2.13 to 3.2.1
adding a settings.gradle file to define the project name

these changes make it possible to use htsjdk in a composite gradle build with other projects

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.003%) to 70.544%
Details
db57898
@superbobry
Contributor

superbobry commented Dec 25, 2016

Hmm, I'm surprised GH also pulled in the Gradle commit.
@coveralls

coveralls commented Dec 25, 2016

Coverage Status

Coverage decreased (-0.003%) to 70.544% when pulling db57898 on superbobry:get-record-and-positions into c795101 on samtools:master.
@yfarjoun
Contributor

yfarjoun commented Dec 28, 2016

@lbergelson can you take a look here? I'm not sure why there are gradle-related changes here....
@lbergelson
Contributor

lbergelson commented Jan 4, 2017

So, I think that what happened here is that @superbobry rebased onto master, but accidentally squashed his commit into the last commit that was already on master. I think we're seeing a display issue on github where it's showing the actual commit diffs, not the diff between master and this commit. Merging should be safe, but the authorship will be screwed up. I think it's going to list me as the author and not suprbobry.

@superbobry If you want to fix it, I think you can go in and manually --amend your commit to unstage the changes to the gradle files and fix the author to be you. Then rebase on top of master again. If you don't care, let me know and I can merge them in as is.
@superbobry
Contributor

superbobry commented Jan 4, 2017

@lbergelson frankly, I don't care enough to go through amend and rebase, so feel free to merge this as is. Thanks!

@lbergelson lbergelson merged commit 1dd11be into samtools:master Jan 4, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 70.544%
Details
@lbergelson
Contributor

lbergelson commented Jan 4, 2017

@superbobry Merged. Github managed to figure out the authorship issue automatically somehow, so it is represented correctly as your contribution. Thanks for the patch and sorry for the unnecessary pain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment