Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

use a locale-independent comparison #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

notslang
Copy link

It's a little faster, and is more stable (since it doesn't rely upon the configured locale).

This does result in a slightly different sort order, which seems more correct. For example, with the current code, this is a sorted list:

  • uber
  • vice_gr
  • vice_serbia
  • vice_spain
  • vice
  • vicechina

With the new code, this is sorted:

  • uber
  • vice
  • vice_gr
  • vice_serbia
  • vice_spain
  • vicechina

@jerone
Copy link

jerone commented Aug 20, 2015

Besides speed (arguable, until proven with prefs), what are your arguments to remove the locale comparison?

@notslang
Copy link
Author

Here's a perf: http://jsperf.com/operator-vs-localecompage/14

And the other arguments are:

  • It's more stable (since it doesn't rely upon the configured locale).
  • It results in a slightly different sort order, which seems more correct (see first comment).

@notslang
Copy link
Author

Is there anything else I can do to help this get merged?

@izuzak
Copy link
Contributor

izuzak commented Sep 15, 2015

Is there anything else I can do to help this get merged?

@slang800 Here's a few suggestions:

  1. It seems that this change doesn't break any tests -- that's surprising. In order to prevent future changes which would break the new behavior (if this PR is merged), it would be great to have tests which verify that the sort results is expected with respect to these changes.
  2. A nice thing about version control is that it allows you to go back in time and see who wrote the code you're changing. That allows you to ask that person why they wrote the code like they did and ask if they have any thoughts about the change you're proposing. It would be great if you could do that here and then ping that user and perhaps also the user who made the most changes to the part of code you're changing.

What do you think?

@notslang
Copy link
Author

rebased (someone else fixed the indentation in the time this PR has been open, so that's no longer in the diff)

has slightly better performance, and is more stable (since it doesn't
rely upon the configured locale)
@notslang
Copy link
Author

rebased again

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants