Add block size display to ls (ls -l)
#2488
Conversation
There was a problem hiding this comment.
Looks good! Some abstractions seem to need a little bit of a rework in general with then new features that have been added (including this one), but rewriting that should not be part of this PR. For this PR, I just have 2 nits.
The terminal of the Windows CI has a default width, which is why those tests fail. You could add -w 0 to give everything its own line and match the expected output.
src/uu/ls/src/ls.rs
Outdated
| // - blocks | ||
| fn get_max_sizes (items: &[PathData], config: &Config) -> (usize, usize, usize, u64) { | ||
| let (mut max_links, mut max_width, mut max_blocks) = (1, 1, 1); | ||
| let mut total_size = 0; |
There was a problem hiding this comment.
Please fix this indentation
src/uu/ls/src/ls.rs
Outdated
| if config.format != Format::Long && config.blocks { | ||
| if let Some(md) = path.md() { | ||
| width += max_blocks + 1; | ||
| name.insert_str(0, &format!("{} ", pad_left(get_display_size(md).to_string(), max_blocks))); |
There was a problem hiding this comment.
Maybe we can make pad_left generic over types implementing Display? I think it also makes sense to put the width += max_blocks + 1; outside of the if-else.
|
Thanks @tertsdiepraam! I'm yet to update the tests but have the other items worked out. |
|
Hey @tertsdiepraam do let me know if there's anything else to change here, thanks! |
|
I wanted to merge this, but decided to quickly run this locally and got values that are off by a factor of 2. The From the
From the Rust docs:
Looks good otherwise and I can merge this if you can fix this :) |
|
Sounds good, @tertsdiepraam I tested this on macOS but I'll test it on other platforms. I'm probably going to spend a bit more time and honor the |
|
Hey, unfortunately due to some personal issues, I am unable to continue work on this. I apologize... If anyone wants to take over and fix the issues, or write something else from scratch, please go ahead! Thank you for your time @tertsdiepraam |
|
I hope you are doing well. I think this PR is too good to throw away, so I'll continue this when I have the time (in about a week). In the meantime, someone else could pick this up as well. Thanks for your work on this PR! |
|
Thank you, @tertsdiepraam! Things just got a bit busy here, but hope to contribute more a few weeks down the line. Feel free to make any changes and even rebase the commits as you see fit! Have a good weekend. |
|
Alright! Good luck and have a good weekend! |
This adds block size display to
lson Unix systems, and displays size on Windows to keep it consistent with the existing behaviour. This works on both long and grid-displays. This still does not implement the-kflag for #2257 but we can add it going forward.I've tested this on macOS, Windows and FreeBSD.
I'm aware there's already a PR #2435< 8000 /a> but that has since been closed, so I thought I'd open this instead.