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

Fix FP S2871 (no-alphabetical-sort): Add exception for string arrays #4531

Closed
ilia-kebets-sonarsource opened this issue Jan 25, 2024 · 4 comments · Fixed by #4539
Closed
Assignees
Labels
mmf-2934 https://sonarsource.atlassian.net/browse/MMF-2934
Milestone

Comments

@ilia-kebets-sonarsource
Copy link
Contributor

Do not raise the issue when the array is an array of strings:

const names = ['foo', 'bar'];
names.sort(); // compliant
@ericmorand-sonarsource
Copy link
Contributor

@ilia-kebets-sonarsource, wouldn't adding this exception contradict the documentation and the initial purpose of the rule?

I quote the documentation:

Even to sort strings, the default sort order may give unexpected results. Not only does it not support localization, but it also doesn’t fully support Unicode, as it only considers UTF-16 code units. For example, in the code below, "eΔ" is surprisingly before and after "éΔ".

const code1 = '\u00e9\u0394'; // "éΔ"
const code2 = '\u0065\u0301\u0394'; // "éΔ" using Unicode combining marks
const code3 = '\u0065\u0394'; // "eΔ"
console.log([code1, code2, code3].sort()); // Noncompliant: ["éΔ", "eΔ", "éΔ"], "eΔ" position is inconsistent
console.log([code1, code2, code3].sort((a, b) => a.localeCompare(b))); // ["eΔ", "éΔ", "éΔ"]

This case would not trigger the rule anymore if we add the exception. Is it desirable? Should I update the documentation accordingly?

@ilia-kebets-sonarsource
Copy link
Contributor Author

  • when we have an array of strings, message: use string.localeCompare(string)
  • update RSPEC to make this suggestion more explicit

@ericmorand-sonarsource
Copy link
Contributor

@ilia-kebets-sonarsource

Currently, the message is the same across all cases (arrays of strings, heterogeneous arrays...): "Provide a compare function to avoid sorting elements alphabetically."

Only the suggestions differ. For arrays of strings, the suggestion is "Add a comparator function to sort in ascending language-sensitive order."

For arrays of strings, I propose changing the emitted message to something like "Provide a compare function that depends on string.localeCompare, to reliably sort elements alphabetically." or "Provide a compare function that reliably sorts elements alphabetically." - ideas more than welcome.

The suggestion already looks good to me.

@ericmorand-sonarsource
Copy link
Contributor

ericmorand-sonarsource commented Feb 1, 2024

Successfully checked on Next for arrays of strings.

image

However, we don't have any code sample that would trigger the issue for heterogeneous arrays so I couldn't check the rule message for this case.

@yassin-kammoun-sonarsource yassin-kammoun-sonarsource added this to the 10.12 milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mmf-2934 https://sonarsource.atlassian.net/browse/MMF-2934
Projects
None yet
3 participants