Skip to content

ls: ignore leading period when sorting by name#2112

Merged
sylvestre merged 4 commits intouutils:masterfrom
ricardoaiglesias:ls-fix-sortnames
Apr 25, 2021
Merged

ls: ignore leading period when sorting by name#2112
sylvestre merged 4 commits intouutils:masterfrom
ricardoaiglesias:ls-fix-sortnames

Conversation

@ricardoaiglesias
Copy link
Contributor

ls now behaves like GNU ls with respect to sorting files by ignoring
leading periods when sorting by main.

Added tests to ensure "touch a .a b .b ; ls" returns ".a a .b b"

Note: The non src/ls/ files that were modified were modified because of clippy warnings that wouldn't let me commit otherwise.

@ricardoaiglesias
Copy link
Contributor Author

Every check except stable x86_64-unknown-freebsd-12 passes. The failing check fails on cp, which was not modified during this PR. I think it's ready to merge.
Thoughts?

Sort::Name => entries.sort_by_key(|k| k.file_name.to_lowercase()),
Sort::Name => entries.sort_by_key(|k| {
let has_dot: bool = k.file_name.starts_with('.');
let filename_nodot: String = if has_dot {
Copy link
Contributor

Choose a reason for hiding this comment

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

Names are usually short so this maybe a nitpick, but we could work with something like this that does not involve clone / collect:

let filename_nodot = &k.file_name[if has_dot { 1 } else { 0 }..];

Copy link
Contributor

Choose a reason for hiding this comment

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

And please add a test to cover this case :)

Copy link
Contributor Author

@ricardoaiglesias ricardoaiglesias Apr 25, 2021

Choose a reason for hiding this comment

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

I think I did the clone/collect thing because I was thinking about the general case of "characters in Rust can be up to four bytes long", but I guess if we know the first character is an ASCII ., then we're free to slice it.

Also, the test case for the "Dot / No Dot" sorting is in test_ls.rs (inside the test_ls_sort_name function). Should I add more tests than the simple test case I have right now? (Nevermind. Just saw that these two lines aren't covered... I can try to change that).

Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

Looks great :)

// until then, the below operates as an outline
// of how it would work.
let result: Vec<u8> = vec![0];
let _result: Vec<u8> = vec![0];
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this unrelated change :)

Sort::Name => entries.sort_by_key(|k| k.file_name.to_lowercase()),
Sort::Name => entries.sort_by_key(|k| {
let has_dot: bool = k.file_name.starts_with('.');
let filename_nodot: String = if has_dot {
Copy link
Contributor

Choose a reason for hiding this comment

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

And please add a test to cover this case :)

ls now behaves like GNU ls with respect to sorting files by ignoring
leading periods when sorting by main.

Added tests to ensure "touch a .a b .b ; ls" returns ".a  a  .b  b"
@sylvestre sylvestre merged commit c3d7358 into uutils:master Apr 25, 2021
@ricardoaiglesias ricardoaiglesias deleted the ls-fix-sortnames branch April 25, 2021 20:40
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.

3 participants