[Question] Units used for disk space

[Rolv-Apneseth]

So, I have code working for macchina to use the disk_space function from here, but in formatting it for display there, I need to cast the u128 values as u64 i.e.:

    let total_kb = ByteSize::b(total as u64);
    let used_kb = ByteSize::b(used as u64);

This seems to not be consistent with the memory readout, which just returns u64 values, is that intentional?

Also, I believe the description of the function is maybe wrong, as the return type is Result<(u64, u64), ReadoutError>:

/// This function should return the used disk space in a human-readable and desirable format.

Can I make a couple changes to the things I pointed out above and make a PR for it?

[grtcdr]

This seems to not be consistent with the memory readout, which just returns u64 values, is that intentional?

Good question.

It seems that the choice of 128 bits rather than the dynamically determined 32 bit or 64 bit long integer

https://github.com/Macchina-CLI/libmacchina/blob/2bea1e0c22fc5e9ec491830bdc7976e891a3b0c1/src/shared/mod.rs#L252-L255

is simply an artifact of age.

When the disk space readout was first introduced, @123marvin123 made the choice of returning a byte_unit and byte_unit::Byte::from_bytes takes as a parameter a 128 bit integer. Sometime after, I removed this dependency because it seemed better to return builtin types rather than "wrapper" types. This means that the cast is unnecessary now, so thanks for pointing this out.

Can I make a couple changes to the things I pointed out above and make a PR for it?

Yes, that would be great!

[Rolv-Apneseth]

Alright, I'll make a PR and hopefully you can point out if I made a mistake (not sure exactly how this stuff works tbh)

[grtcdr]

Thanks a lot, will do.

[Rolv-Apneseth]

Alright that's the PR, hopefully I didn't miss anything. I have the code for macchina to show disk space ready too once everything is approved and the version is updated. Thanks for your time reviewing the code.