-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: adds fallbackSort
option to all rules
#457
Conversation
fallbackSort
optionfallbackSort
option to all rules
da2686b
to
efca022
Compare
efca022
to
7ef7972
Compare
7ef7972
to
365f0ce
Compare
{ | ||
type: 'line-length', | ||
order: 'desc' | ||
fallbackSort: [ |
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.
Do you think it should be an array? Why not object?
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.
@azat-io 👍 I changed it to an object here: 4d4b494
(#457)
At first, I wanted an array because we might need to define multiple fallback sorts later, if we implement #434 for example (users might want to sort by usage > sort by line-length > sort by alphabetical order
, in which case we would need an array).
But today, we don't need it yet, so it makes sense to directly use an object. We can later allow fallbackSort: { type: string; order?: 'asc' | 'desc' } | { type: string; order?: 'asc' | 'desc' }[]
if needed.
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.
The examples in the documentation use arrays ^
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.
[ | ||
'aa', | ||
'b', | ||
'c', |
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 like it.
But I think the tests would be better if we also checked that sorting by line length works and is prioritized.
{
code: dedent`
[
'c',
'bbb',
'a',
].includes(value)
`,
output: dedent`
[
'bbb',
'a',
'c',
].includes(value)
`,
}
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.
Nice feature, thank you! |
Resolves #429
Most of the code added are documentation and tests.
The two commits responsible for the implementation are:
204ef1e
(#457)6486253
(#457)Description
This PR adds a
fallbackSort
option to all rules:Typings
This option is currently useful for the
line-length
sort type: this enables users to define an additional sorting algorithm when two elements have the same length.The option allows an array in case additional sorting types which can result in elements being equals are added later (such as
sorting by usage
).What is the purpose of this pull request?