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

Incorrect Array sortBy order #382

Closed
Mottie opened this issue Oct 14, 2013 · 17 comments
Closed

Incorrect Array sortBy order #382

Mottie opened this issue Oct 14, 2013 · 17 comments

Comments

@Mottie
Copy link

Mottie commented Oct 14, 2013

Once again, thank you for an awesome library!

First off, the sort tests appear to be failing, but I can't tell if it is a problem with the sortBy function.

So, even with the default sort order & ignore case set to false:

Array.AlphanumericSortOrder = "AÁÀÂÃĄBCĆČÇDĎÐEÉÈĚÊËĘFGĞHıIÍÌİÎÏJKLŁMNŃŇÑOÓÒÔPQRŘSŚŠŞTŤUÚÙŮÛÜVWXYÝZŹŻŽÞÆŒØÕÅÄö";
Array.AlphanumericSortIgnoreCase = false;

If I sort an identical sort order string, I get this result:

"AÁÀÂÃĄBCĆČÇDĎÐEÉÈĚÊËĘFGĞHıIÍÌİÎÏJKLŁMNŃŇÑOÓÒÔPQRŘSŚŠŞTŤUÚÙŮÛÜVWXYÝZŹŻŽÞÆŒØÕÅÄö".split("").sortBy().join("");

// result
// "AÀÄÂÁÃBCÇDÅÐEÉÈÊËFGHIÏÍÎÌJKLMNOÔÕÒÓPQRSTUWXYØÚVÑÜÛÙÝZÞÆöĄĆČĎĘĚĞİıŁŃŇŒŘŚŞŠŤŮŹŻŽ"

And here is a demo I was using for testing.

@andrewplummer
Copy link
Owner

Tests appear passing to me... maybe this is a locale issue?
Can you let me know your exact environment and locale?

@Mottie
Copy link
Author

Mottie commented Nov 6, 2013

Hi!

I'm on a Windows 8 Pro x64 machine with the region set to United States.

Here are screen shots of the errors I'm seeing on that page:

Chrome 30.0.1599.101 m

error1
error2

Firefox 25.0

error3-firefox

@andrewplummer
Copy link
Owner

Interesting........ can you clear your cache and try again??

@Mottie
Copy link
Author

Mottie commented Nov 6, 2013

Hmm, well, now the demo seems to be working properly, so I'll go ahead and close this issue. Thanks!

@Mottie Mottie closed this as completed Nov 6, 2013
@andrewplummer
Copy link
Owner

Looks like I need to put a cache killer in the iframe URL probably... I'll make a note to do that in the future!
Thanks!

@Mottie
Copy link
Author

Mottie commented Nov 6, 2013

I just cleared my browser cache (Chrome) and reran those tests and I'm still seeing the array error, but not the date one.

@andrewplummer
Copy link
Owner

ok kind of a pain, but can you clone the repo and run test/default.html?
I don't think you'll be able to run it via file:/// so if you have some kind of server set up that would be ideal.

@Mottie
Copy link
Author

Mottie commented Nov 6, 2013

I had to run it in Firefox and load jQuery using http:// instead of // but it worked and showed these errors:

localerror

@andrewplummer
Copy link
Owner

Very interesting... what's the version of FF?

@Mottie
Copy link
Author

Mottie commented Nov 6, 2013

The latest v25.0

@andrewplummer
Copy link
Owner

ok I'll have a look later on a vbox... thanks again!

@Mottie
Copy link
Author

Mottie commented Nov 20, 2013

Hmm, ok I was wrong. The sort is still incorrect.

I updated the demo to only show one comparison... the result should match the sort string.

We start with this sorted string:

AÁÀÂÃĄBCĆČÇDĎÐEÉÈĚÊËĘFGĞHıIÍÌİÎÏJKLŁMNŃŇÑOÓÒÔPQRŘSŚŠŞTŤUÚÙŮÛÜVWXYÝZŹŻŽÞÆŒØÕÅÄö

and should end with the same result, but instead get this:

ABCDEFGHIİJKLMNOPQRSTUVWXYZÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕöØÙÚÛÜÝÞĄĆČĎĘĚĞıŁŃŇŒŘŚŞŠŤŮŹŻŽ

@Mottie Mottie reopened this Nov 20, 2013
@andrewplummer
Copy link
Owner

Hi, I'm really sorry to take so long to respond here!
This got put on the back burner and I never got to it :(

Please have a look here: http://sugarjs.com/arrays#sorting at the second to last paragraph.

Basically you'll have to do a few modifications to achieve correct sort order for Scandinavian languages. You have to turn off the default sort equivalents. For Scandinavian languages only a subset need to be disabled but this is the easiest way:

Array.AlphanumericSortEquivalents = {};

Doing this will allow Array.AlphanumericSortOrder to be respected without any overriding.
I've updated the demo to do this so that it works for you:

http://jsfiddle.net/tBHqG/3/

One more thing though is that the string you passed in for AlphanumericSortOrder only has upper-case characters even though you have IgnoreCase on, so I disabled that to use the defaults which have both upper and lower case.

@Mottie
Copy link
Author

Mottie commented Apr 28, 2014

Awesome! Thanks!

@Mottie Mottie closed this as completed Apr 28, 2014
@Mottie
Copy link
Author

Mottie commented Apr 28, 2014

Oops, I'm still seeing failing tests in both Chrome & Firefox.

Chrome 34.0.1847.131 m (Windows 8 x64)

2014-04-28 07_51_16-unit tests - sugar
2014-04-28 07_51_38-unit tests - sugar
2014-04-28 07_52_05-unit tests - sugar

Firefox 28.0 (Windows 8 x64)

2014-04-28 07_52_45-unit tests - sugar

2014-04-28 07_55_20-unit tests - sugar

@andrewplummer
Copy link
Owner

After looking into it, two were badly formed tests that have since been fixed, and one was a change caused by a Chrome update. This has also been accounted for in the current edge version.
I went ahead and fixed it on the site.

Thanks!

@Mottie
Copy link
Author

Mottie commented Apr 29, 2014

Awesome work!

@Mottie Mottie mentioned this issue Aug 18, 2016
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

No branches or pull requests

2 participants