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

sort -h friendly output mode #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wookietreiber
Copy link

As of now an extremely ugly variant to get in #14. Works, though. Please review.

@wookietreiber
Copy link
Author

@hyunsik The Travis failure is due to:

$ cargo fmt --all -- --write-mode=diff
Unrecognized option: 'write-mode'

@hyunsik
Copy link
Collaborator

hyunsik commented Sep 30, 2018

Hi @wookietreiber,

I'm sorry for a late response. I missed your issue and this PR from a lot of email deluge. I'll fix the travis issue and review your PR by tomorrow. Thank you for your contribution!

@hyunsik
Copy link
Collaborator

hyunsik commented Oct 2, 2018

I've also fixed the build error at 9d8e13f.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@wookietreiber
Copy link
Author

Thanks for catching the println statements. Rebased.

@wookietreiber
Copy link
Author

At the moment, I added a new function to_simple_string_as to ByteSize and a new argument to the to_string stand-alone function. Note that this can be done differently, e.g. adding the simple: bool argument to the to_string_as function on ByteSize and dropping the to_simple_string_as. Both of these variants, though, have implications regarding compatibility and would require a semver bump to the next major version.

It could also be done completely compatible by creating a new stand-alone to_string_simple function as well. But this wouldn't be as clean anymore.

I don't know which way you'd prefer. Please advise.

@hyunsik
Copy link
Collaborator

hyunsik commented Oct 30, 2018

Hi,

I'm sorry for a late response. The simple representation seems to lose a unit of measurement. I think that your approach is a good option.

I also have another suggestion that implement functions to result in float values in different unit of measurements. For example,

pub fn as_mb(&self) -> f64 {
      ...
    }

    pub fn as_mib(&self) -> f64 {
      ...
    }

    pub fn as_gb(&self) -> f64 {
      ...
    }

    pub fn as_gib(&self) -> f64 {
      ...
    }
...

Then, this approach will allows users to handle more various use-cases including your case. What do you think about that?

@wookietreiber
Copy link
Author

wookietreiber commented Dec 8, 2018

I guess, I would prefer an even higher-level approach:

enum OutputFormat {
    SI,   // 1000, e.g. "32 KB"
    IEC,  // 1024, e.g. "32 KiB"
    Sort, // 1024, e.g. "32K"
}

fn to_string(b: ByteSize, fmt: OutputFormat) -> String {
    // ...
}

it's "a utility", not "an ...", because the pronunciation of "utility"
starts with "ju"
@wookietreiber
Copy link
Author

@hyunsik I've finally gotten around to implementing my idea from my last comment.

- this clarifies the intend beyond of what the previously used si
  boolean can
- users may specify their own formatters
- adds a byte formatter that is compatible with `sort -h`

fixes bytesize-rs#14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants