-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fix for navigation past end of chromosome #1719
Conversation
4e576cb
to
0c0422a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1719 +/- ##
==========================================
+ Coverage 58.81% 58.82% +0.01%
==========================================
Files 448 448
Lines 20754 20760 +6
Branches 4919 4922 +3
==========================================
+ Hits 12206 12212 +6
Misses 8241 8241
Partials 307 307
Continue to review full report at Codecov.
|
looks ok to me, but I'm not all that up on LGV navigation. @teresam856 what do you think of this, and of @cmdcolin comment that navigation probably needs some refactoring but anyway, ok with me to merge once @teresam856 weighs in on it |
I agree. I think navigation handles a lot of different cases right now. The changes lgtm but like Colin mentioned, I think a refactor may be needed to break up those actions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #1718
This adds a redundancy that avoids an all out crash, and then fixes the navigation so it clamps to the end of the newly navigating displayed region
This is just a general observation but our logic for navigating is fairly complex...I think it is likely that a refactoring probably along the time that name indexing comes around is likely in order