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

df: fix rounding behavior in humanreadable mode #3554

Merged
merged 1 commit into from
May 25, 2022

Conversation

cakebaker
Copy link
Contributor

@cakebaker cakebaker commented May 22, 2022

This PR makes the following changes:

  • addition of SuffixType enum (replaces the suffix constants)
  • usage of a single to_magnitude_and_suffix function (instead of three)
  • removal of number_prefix dependency

Fixes #3422.

@@ -220,56 +208,64 @@ mod tests {

use std::env;

use crate::blocks::{to_magnitude_and_suffix, BlockSize};
use crate::blocks::{to_magnitude_and_suffix, BlockSize, SuffixType};

#[test]
fn test_to_magnitude_and_suffix_powers_of_1024() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to admit, it's pretty difficult to follow the logic of HumanReadable, SizeFormat, BlockSize, SuffixType, and more, but if I'm reading this pull request correctly, we are just decoupling the value of the number (1024, 2048, 4096, etc.) from the decision of what suffix to apply (SuffixType::IEC, SuffixType:SI, etc.). So we could now call to_magnitude_and_suffix(1024, SuffixType::SI) if we wanted to. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct.

Your comment made me rethink the enums you mentioned and I realized that SizeFormat is not necessary and I turned it into an Option<HumanReadable> with the latest push. I hope this reduces some of the complexity.

@cakebaker
Copy link
Contributor Author

I'm not sure about the upper_case_acronyms lint (https://rust-lang.github.io/rust-clippy/master/index.html#upper_case_acronyms). I currently disabled it. Somehow it looks inconsistent to have Iec and HumanReadableIEC, and renaming it to HumanReadableIec looks a bit strange.

@cakebaker cakebaker force-pushed the ticket_3422 branch 2 times, most recently from 0265afa to e565b52 Compare May 23, 2022 12:58
Copy link
Collaborator

@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

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

Fixes the bug, and removing one of the enums seems like an improvement to me.

@sylvestre sylvestre merged commit 1867b65 into uutils:main May 25, 2022
@cakebaker cakebaker deleted the ticket_3422 branch May 25, 2022 07:25
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.

df -H and df -h have a different rounding behavior than GNU df
3 participants