Skip to content
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(expr): Use a sensible sorting function for sorted() #1990

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Jun 9, 2024

Array.sort(), by default, converts numbers to unicode strings and sorts them lexically. This uses (a > b) - (a < b) to implement a standard comparison operator. Greater results in 1, equal in 0, lesser in -1.

@effigies effigies force-pushed the fix/sorted-function branch from 8ec7265 to 22ea192 Compare June 9, 2024 02:56
Copy link

codecov bot commented Jun 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (162faeb) to head (ee9032c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1990      +/-   ##
==========================================
+ Coverage   85.68%   87.11%   +1.43%     
==========================================
  Files          91      130      +39     
  Lines        3792     6304    +2512     
  Branches     1220     1520     +300     
==========================================
+ Hits         3249     5492    +2243     
- Misses        457      721     +264     
- Partials       86       91       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies effigies added bug impact: high Estimated high impact task labels Jun 9, 2024
@effigies effigies force-pushed the fix/sorted-function branch from 22ea192 to 42bf09f Compare June 10, 2024 01:51
Array.sort(), by default, converts numbers to strings and compares them
lexically. This uses `(a > b) - (a < b)` to implement a standard
comparison operator. Greater results in 1, equal in 0, lesser in -1.
@effigies effigies force-pushed the fix/sorted-function branch from 42bf09f to ee9032c Compare June 10, 2024 02:31
@effigies
Copy link
Collaborator Author

I added sorted([9, 81, 729, 6561]) to the expression tests upstream. This PR fixes it:

  sorted([3, 2, 1])                      1,2,3         1,2,3         pass  
  allequal(sorted([3, 2, 1]), [1, 2, 3]) true          true          pass  
  sorted([9, 81, 729, 6561])             9,81,729,6561 9,81,729,6561 pass  

@nellh nellh merged commit 07cbca5 into bids-standard:master Jun 10, 2024
31 of 33 checks passed
@effigies effigies deleted the fix/sorted-function branch June 10, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug impact: high Estimated high impact task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants