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

Add sorting function to sort dash command above normal one #106

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

yathomasi
Copy link
Contributor

Everything looks good on generated commands list #100 except for order on dashed commands. So, I have added a sorter to move dashed commands higher for proper syntax highlighting.

@yathomasi yathomasi requested a review from a team November 8, 2022 05:23
@yathomasi yathomasi self-assigned this Nov 8, 2022
@rogermparent rogermparent temporarily deployed to gatsby-theme-add-sortin-fwoz96 November 8, 2022 05:23 Inactive
Comment on lines +15 to +18
const sortDashedCommand = (a, b) => {
if (a.split('-')[0] === b) return -1
return 0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this sort function looked funny, so I stuck it in the Node REPL and tried it out. It works when the dashed commands are next to each other:

> ['abc', 'ab', 'ab-c', 'abcd'].sort(sortDashedCommand)
[ 'abc', 'ab-c', 'ab', 'abcd' ]

but not when there is anything in between:

> ['ab', 'abc', 'ab-cd', 'ab-c'].sort(sortDashedCommand)
[ 'ab', 'abc', 'ab-cd', 'ab-c' ]

I get what this is going for, and I think given the order we usually give it this will work, but we may want a more guaranteed sort here that explicitly puts commands with - above all others, or if we want to keep the aesthetics of grouped commands we could use a reducer that collects dashed and non-dashed commands, putting together a list ensuring they're grouped by first segment but the non-dashed base is last in each group.

That said, it's better than nothing, so I wouldn't consider it a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to fix the particular case that the dashed command are next to each other for now. So, if we encounter another case scenario you mentioned later, we can come up with a fix.

@yathomasi yathomasi merged commit 797d014 into main Nov 8, 2022
@yathomasi yathomasi deleted the add-sorting-to-commands-script branch November 8, 2022 16:02
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