-
Notifications
You must be signed in to change notification settings - Fork 256
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
Make PageText.sortPosition() sort order deterministic. #153
Conversation
Codecov Report
@@ Coverage Diff @@
## development #153 +/- ##
==============================================
Coverage ? 57.14%
==============================================
Files ? 187
Lines ? 33719
Branches ? 0
==============================================
Hits ? 19270
Misses ? 11954
Partials ? 2495
Continue to review full report at Codecov.
|
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.
Seems to me like the previous algorithm should work with small change. That one is preferable due to its simplicity. If that is not the case, the rationale for the added complexity should be clarified.
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
This is a fix for #152
The sort order is now deterministic and the ordering is explained in the source code.
A test case shows how the sorting works.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)