-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bug: [member-ordering] natural-sort should be implemented using String's native capabilities instead of a 3rd party package #5989
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
Comments
Interesting! Which native API are you referring to? |
ooops, sorry, I should have mentioned it here. I mentioned it in the message that I linked :) I'm refering to the native JS API ["a2", "a10", "a1"].sort((a, b) => a.localeCompare(b, undefined, { numeric: true }))
// Result: [ 'a1', 'a2', 'a10' ] I'm thinking that probably instead of Hope this helps |
Ah thanks - so this actually isn't Node-specific, it's JS strings in general. That threw me - we support running in browsers and therefore try not to rely on Node-specific APIs. I tried the suggested change out locally and saw a couple of incorrect errors. For example:
The Am I missing something? |
I'd like to see your code :) Especially have you used |
// import naturalCompare from 'natural-compare-lite';
function naturalCompare(a: string, b: string): number {
return a.localeCompare(b, 'en-US', { numeric: true });
} We've been strapped for time recently so I'm going to close this. But if you can make a draft PR that shows existing test cases passing with |
My understandng: Normally we can configure this in We can also decide that localeCompare order is the right one -- this is just a choice after all. I asked around to colleagues about the issue though. |
Sounds good! We can always look at an issue for new sort options if you feel there's a variant that should be added. |
I got the bottom of this.
Therefore with |
I left a message on the PR that implemented this rule => #5662 (comment)
But I thought it was better to file a new issue instead.
I saw that #5662 implemented a natural-sort-ordering option, which sounds good. However this can be implemented using node's native API instead of relying on a 3rd-party lib.
Would you be open to a PR for this?
The text was updated successfully, but these errors were encountered: