Skip to content

Fix Sorting of Special Characters #91

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

Closed
wants to merge 4 commits into from
Closed

Conversation

akaustav
Copy link

@akaustav akaustav commented Oct 3, 2021

DRAFT PR WITH INCOMPLETE THOUGHTS - DO NOT MERGE

TODO:

  1. cleanup assumptions to replace based on ASCII order.
  2. cleanup the "hack" reversal (or maybe this is what fixed the problem?? - need to check)
  3. fix the unit test ordering of './/'
  4. fix the unit test ordering of hyphens vs commas (../-a vs ../,)
  5. cleanup the NodeJS 16 changes (or keep it based on your preference)

@lydell
Copy link
Owner

lydell commented Oct 7, 2021

Consider these two lines:

../-a
../../-a

Both good old character code based sorting and Intl.Collator agree that they are sorted. Both lines start with ../. Then the first differing characters are - and .. - comes first so ../-a “wins”.

Now consider these two lines (- changed to ~):

../~a
../../~a

This time, both good old character code based sorting and Intl.Collator agree that they are not sorted. Both lines start with ../. Then the first differing characters are ~ and .. . comes first so ../../~a “wins”. They need to swap order to be sorted.

Finally, how would this plugin like to sort them? Well, it thinks that files further up the directory tree should come first. So, more ../:es should come first.

As you can see, sometimes (the ~/second example) the desired sorting can be achieved by just sorting as strings. But other times (the -/first) we end up with another order. Why is that?

Well, using both sorting methods, all characters that sort before . and / are … other punctuation. So if all sequences of ../ are followed by non-punctuation (like letters), we’re gonna get the desired order. If all sequences of ../ are followed by punctuation, it depends. If the punctuation sorts after . or / it works. If it sorts before, we don’t get the desired “directory structure” sorting.

This leads to the current solution in this plugin. It makes sure that . and / sort first of all (by using clever temporary character substitutions).

If we’re going to stay with that approach, I don’t think it’s possible to tweak the substitutions like you have in this PR. This approach has to sort . before -, otherwise the first example won’t sort as expected:

// Expected order:
import b from "../../-a";
import a from "../-a";

I realize that might be missing from the test suite.

@lydell
Copy link
Owner

lydell commented Oct 9, 2021

See #87 (comment).

@lydell lydell closed this Oct 9, 2021
@akaustav
Copy link
Author

akaustav commented Oct 9, 2021

@lydell - I apologize, I was busy the whole week with other high priority items, so couldn't get to this issue / PR in time. Thanks for taking the time to put together the explanation. I understand and agree that imports from outer level parents should take higher priority than the inner-level parents. Therefore imports from ../../xyz comes before ../xyz. However, what about the following scenario when both imports are from the same level (../ in this case)?

import y from '../a.b';
import x from '../a-b';

In this case both good old character code based sorting and Intl.Collator based sorting agree that they are NOT sorted.

// Character code based sorting
['../a.b', '../a-b'].sort();
// Output: [ '../a-b', '../a.b' ]

// Intl.Collator based sorting
['../a.b', '../a-b'].sort(new Intl.Collator().compare);
// Output: [ '../a-b', '../a.b' ]

The "from" strings on both lines start with ../a. Then the first differing characters are period (.) and hyphen (-). In this case hyphen (-) should come first - irrespective of character code based sorting or Intl.Collator based sorting. After sorting, the output should be this in my opinion:

import x from '../a-b';
import y from '../a.b';

However, in the current state of this plugin, ../a.b “wins” and generates this output:

import y from '../a.b';
import x from '../a-b';

This was the main problem that I was trying to describe in issue #87.
And I also understand that this is not trivial to “fix” - based on the regular expression based replacements that have been done within the getSource() method in src/shared.js.

@akaustav
Copy link
Author

akaustav commented Oct 9, 2021

@lydell - Sorry, I posted my comments above before reading the updated README.md file.

Intl.Collator sorts punctuation in some defined order. I have no idea what order punctuation sorts in, and I don’t care. There’s no ordered “alphabet” for punctuation that I know of.

Since you have decided you don't care - I will stop bothering you. Thank you for your time.

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.

2 participants