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
): Rework the rule to emit a dedicated message for arrays of strings
#4539
Conversation
18accf6
to
cb85a97
Compare
@@ -53,6 +53,8 @@ export const rule: Rule.RuleModule = { | |||
messages: { | |||
provideCompareFunction: | |||
'Provide a compare function to avoid sorting elements alphabetically.', | |||
provideCompareFunctionForArrayOfStrings: | |||
'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.', | |
'Provide a compare function that depends on "String.localeCompare", to reliably sort elements alphabetically.', |
@@ -606,6 +606,8 @@ ruleTester.run( | |||
code: 'const array = ["foo", "bar"]; const sortedArray = array.toSorted();', | |||
errors: [ | |||
{ | |||
message: | |||
'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to apply this suggestion if you applied the one I suggested above.
'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.', | |
'Provide a compare function that depends on "String.localeCompare", to reliably sort elements alphabetically.', |
<h3>Exceptions</h3> | ||
<p>The rule ignores arrays that only contain strings, or are declared as such.</p> | ||
<pre> | ||
const names = ['foo', 'bar']; | ||
names.sort(); // Compliant | ||
</pre> | ||
<pre> | ||
const names: string[] = []; | ||
names.sort(); // Compliant | ||
</pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the exception no longer holds and we can get rid of this from the RSPEC, right?
We could also update the PR's title since it's no longer about adding an exception for string arrays but rather having a specific message for string arrays. |
cb85a97
to
24b3623
Compare
Quality Gate passedIssues Measures |
no-alphabetical-sort
): Add exception for string arraysno-alphabetical-sort
): Rework the rule to emit a dedicated message for arrays of strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #4531