-
-
Couldn't load subscription status.
- Fork 339
fix: ignore extensions for directories when sorting #1418
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
base: main
Are you sure you want to change the base?
Conversation
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.
Indeed, it makes a lot of sense to change the behavior this way!
|
Hum now that I think of it, it would be great to have some tests for this behavior. You can take 570b977, add it to this PR and see if it works in the CI. |
|
Thanks. Looks like more tests need to be updated? |
Would appreciate it! |
ae526de to
077d6da
Compare
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
|
Hey thanks for the pr, this is indeed a slight issue !! |
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 catch! Thanks! ... Code, functionality and performance looks good to me!
|
@cafkafk could you give this another look? Would be nice to get this into a release and help me remove my local nix override. |
|
Not sure what the next steps are to get this merged. |
|
Well, we’d have to wait for @cafkafk to be available to review that but idk when that’ll be 😭 |
The merge-base changed after approval.
f96bc8d to
788a8d9
Compare
|
Hi all. I seemed to have missed the latest comment by @cafkafk (no Github email?). Anyway I have cherry picked my change to the latest main. I tried following the instructions to regenerate the tests. However, running @ariasuni /@cafkafk: can you help regenerate the tests and get this finally merged? |
Before this patch:
Note how
01.cityis sorted after02.applewhen sorting by extension.After patch:
Directories are treated exactly as if there are no
.was found in the name.Closes #821