Fixing sorting of directory and filenames with numbers #18539

Merged
merged 2 commits into from Jan 16, 2017

Projects

None yet

4 participants

@jchadwick
Contributor
jchadwick commented Jan 14, 2017 edited

These changes replace the existing regex-based sorting of filenames and directory names with a call to the String.prototype.localeCompare() function which supports this natively via the numeric option.

Also added some unit test cases for filenames with numbers.

CC: @bpasero

Fixes #17495

@bpasero
Member
bpasero commented Jan 14, 2017

@jchadwick incredible, I was not aware of these options. Thanks for enlighten me ๐Ÿ‘

@bpasero bpasero self-assigned this Jan 14, 2017
@paror
paror commented Jan 14, 2017

I read this on MDN about String.prototype.localeCompare(): When comparing large numbers of strings, such as in sorting large arrays, it is better to create an Intl.Collator object and use the function provided by its compare property.

@jchadwick
Contributor

yeah I saw that too but it wasn't obvious how to do that here. I can take a stab, though.

@jchadwick
Contributor
jchadwick commented Jan 14, 2017 edited

Wow. Intl.Collator is significantly (97%) faster! (And I've switched to it)

jchadwick added some commits Jan 14, 2017
@jchadwick jchadwick Fixing sorting of directory and filenames with numbers (fixes #17495) 2bb0cf9
@jchadwick jchadwick Switching from String.localeCompare() to Intl.Collator
afc5448
@paror
paror commented Jan 15, 2017

Nice!

@bpasero bpasero added this to the February 2017 milestone Jan 16, 2017
@bpasero
Member
bpasero commented Jan 16, 2017

LGTM

@bpasero bpasero merged commit 7a949d8 into Microsoft:master Jan 16, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@alexandrudima alexandrudima referenced this pull request in Microsoft/monaco-editor Jan 21, 2017
Closed

Can't find variable: Intl error on Safari #327

@jchadwick
Contributor

Hey @bpasero, just an FYI, this wasn't attributed to me in the 1.9 release notes.

@bpasero
Member
bpasero commented Feb 3, 2017

@jchadwick sorry for that, I wrongly set this PR for February and so it was not picked up by our automated tool that generates release notes for PRs. I will add this, but it might take a day or two until our website gets redeployed.

@bpasero
Member
bpasero commented Feb 3, 2017

Added via Microsoft/vscode-docs@229ceeb so it should be in the next website update.

@jchadwick
Contributor

@bpasero No worries. Thanks!

@jchadwick jchadwick deleted the jchadwick:jchadwick/explorer-sorting-issue branch Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment