Skip to content

update pelias/sorting module to fix numeric sorting bug#1626

Merged
missinglink merged 2 commits intomasterfrom
fix-numeric-sorting-bug
Jun 20, 2022
Merged

update pelias/sorting module to fix numeric sorting bug#1626
missinglink merged 2 commits intomasterfrom
fix-numeric-sorting-bug

Conversation

@missinglink
Copy link
Copy Markdown
Member

this PR fixes a subtle sorting bug: pelias/sorting#26

in particular, a user noted that the query Acton, ME, USA returned the result for Meeker County above that of Maine, which is now resolved.

@missinglink missinglink requested a review from orangejulius June 20, 2022 11:55
@missinglink missinglink force-pushed the fix-numeric-sorting-bug branch from 035d0a0 to 9998ba6 Compare June 20, 2022 15:29
@missinglink
Copy link
Copy Markdown
Member Author

missinglink commented Jun 20, 2022

I've added a second commit which removes the stable module since stable sort has been available natively since node@12.

Note: the comparator is expected to return a finite number, I hunted for any cases where it might return a boolean and couldn't find any in the code, one in the tests, worth keeping an 👁️ out for.

@missinglink
Copy link
Copy Markdown
Member Author

acceptance tests look good, there was one regression:

/v1/search?sources=wof&text=Taman Taynton View, Malaysia

# previously
0) Taman Taynton View, Kuala Lumpur, Malaysia
1) Taman Taynton, Kuala Lumpur, Malaysia

# now
0) Taman Taynton, Kuala Lumpur, Malaysia
1) Taman Taynton View, Kuala Lumpur, Malaysia

This is due to the results being displayed in order returned from pelias/placeholder.

I'm happy to merge this as-is, we can tackle the sorting order returned by placeholder in a subsequent PR.

@missinglink missinglink merged commit 10d56c5 into master Jun 20, 2022
@missinglink missinglink deleted the fix-numeric-sorting-bug branch June 20, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant