-
Notifications
You must be signed in to change notification settings - Fork 43
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
🐛 Use natural sort order #1918
🐛 Use natural sort order #1918
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1918 +/- ##
==========================================
+ Coverage 39.20% 42.13% +2.93%
==========================================
Files 146 163 +17
Lines 4857 5236 +379
Branches 1164 1325 +161
==========================================
+ Hits 1904 2206 +302
+ Misses 2939 2913 -26
- Partials 14 117 +103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Look really good, just a handful of questions
Replace default alphabetical sort order (i.e. a, a11, a2) with human friendly "natural" sort (i.e. a, a2, a11). The change applies to all found cases of string sorting. For convenience the comparator converts all values to strings (with nulish values being converted to empty string). In addition, the natural sort with string conversion (universal comparator) was used as a replacement in sorting hooks. There a general sorting algorithm was implemented with type-specific comparators. Benefits of universal comparator: 1. better handling of mixed types (i.e. string and number). String value most likely gets converted to NaN. 2. better handling of undefined (numeric value NaN) 2. simplified coercion logic compared to less than/greater then operators Reference-Url: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Less_than Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
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
Replace default alphabetical sort order (i.e. a, a11, a2)
with human friendly "natural" sort (i.e. a, a2, a11).
The change applies to all found cases of string sorting.
For convenience the comparator converts all values to strings (with
nulish values being converted to empty string).
In addition, the natural sort with string conversion (universal
comparator) was used as a replacement in sorting hooks. There a general
sorting algorithm was implemented with type-specific comparators.
Benefits of universal comparator:
most likely gets converted to NaN.
operators
Note that one functional change was introduced in the sorting hook:
space removal logic was dropped as it creates visually confusing results
for stable sorting.
Resolves: #1796
Reference-Url: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Less_than