Skip to content

Implement Total size feature#2170

Merged
sylvestre merged 4 commits intouutils:masterfrom
siebenHeaven:total_size_feature
May 5, 2021
Merged

Implement Total size feature#2170
sylvestre merged 4 commits intouutils:masterfrom
siebenHeaven:total_size_feature

Conversation

@siebenHeaven
Copy link
Contributor

@siebenHeaven siebenHeaven commented May 3, 2021

Total size for directories in long format is implemented with this PR for unix. For non-unix environments, there is no exact equivalent for what GNU displays so I've left it unimplemented at the moment instead of printing some other value.

Partially fixes #1872

@sylvestre
Copy link
Contributor

sylvestre commented May 4, 2021

Fails on Windows


failures:

---- test_ls::test_ls_long_total_size stdout ----
current_directory_resolved: 
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpRgTsbE\test-long
open(append): C:\Users\RUNNER~1\AppData\Local\Temp\.tmpRgTsbE\test-long
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpRgTsbE\test-long2
open(append): C:\Users\RUNNER~1\AppData\Local\Temp\.tmpRgTsbE\test-long2
run: D:\a\coreutils\coreutils\target\x86_64-pc-windows-msvc\debug\coreutils.exe ls -l
thread 'test_ls::test_ls_long_total_size' panicked at 'assertion failed: self.stdout_str().contains(cmp.as_ref())', tests\common\util.rs:298:9

@siebenHeaven
ls: Implement total size feature
6551cbe 
- Implement total size reporting that was missing
- Fix minor formatting / readability nits
@siebenHeaven
tests: Add tests for ls total sizes feature
d3aaf9b
@siebenHeaven siebenHeaven force-pushed the total_size_feature branch from f8b4c3e to d3aaf9b Compare May 4, 2021 15:24
@siebenHeaven
Copy link
Contributor Author

siebenHeaven commented May 4, 2021

@sylvestre Thanks - I'd missed to disable the test for windows.
Current PR only implements this for unix - I don't see a straight-forward way to have this on windows (we could print file size directly but I'd like to hear some opinions on whether we want to go that way)

I also see MSRV failures - I'll do another update to fix those

tertsdiepraam
tertsdiepraam reviewed May 4, 2021
View reviewed changes
Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Nice work! I also like some of the small other changes you made, they look very sensible. I think we can just use md.len() for Windows.

@siebenHeaven
ls: Add windows support for total sizes feature
1fd2734 
- Add windows support (defaults to file size as block sizes related
infromation is not avialable on windows)
- Renamed some functions
@siebenHeaven
Copy link
Contributor Author

siebenHeaven commented May 5, 2021

I've updated to use md.len for windows and added corresponding test.
I've also renamed some functions to more appropriate names.

@sylvestre sylvestre merged commit 7d2b051 into uutils:master May 5, 2021
@siebenHeaven siebenHeaven deleted the total_size_feature branch May 21, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tertsdiepraam tertsdiepraam tertsdiepraam left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ls: Compatibility Tracking Issue

3 participants

@siebenHeaven @sylvestre @tertsdiepraam