Fixed inverted SI conditions (and related things)

[Andrew15-5]

• Conditions that check whether the SI or IEC units must be used were inverted, i.e., when si_prefix == true it would use "iB" instead of "B".
• KB & kiB were used instead of kB & KiB.
• Switches (true/false) in tests are also fixed.
• LN_KIB & LN_KB had swapped values.
• Updated tests in README.md.
• Removed comment about fixed bug in ./src/lib.rs since there are no bugs in the test case: // a bug case: https://github.com/flang-project/bytesize/issues/8
• Rust code now has consistent 4 spaces indents in README.md.

Fixes #25 Fixes #26 Fixed #37

[andrewgazelka]

@Andrew15-5 is there a reason this hasn't been merged in yet? Is this project unmaintained?

[Andrew15-5]

My guess is that the author is very busy, which is, of course, unfortunate, but understandable. This does kind of fall under the definition of unmaintained project.

This is the last message I've seen: https://github.com/hyunsik/bytesize/pull/35#issuecomment-1707472060.

[Andrew15-5]

I want to say that after reworking the recent changes to fix the merge conflicts (and during the initial development of the PR) I faced a lot of issues, due to poor architecture (it's not that bad, but it's not that great either). The https://github.com/hyunsik/bytesize/pull/35 and https://github.com/hyunsik/bytesize/issues/25#issuecomment-820194592 do have a point, that the code base can be improved to be more readable and maintainable.

In particular, I don't like the copied tests from Rust file to Markdown file: you have to keep them in sync, and I haven't found any copy/sync logic in the justfile. It's super easy to mess up the tests in the Markdown file, as they will become either invalid or simply outdated. There are a lot of boilerplates (including a lot of booleans), such as:

Examples

https://github.com/hyunsik/bytesize/blob/5de80fbac0648df8d1bb10fa098f1fad45ef358c/src/lib.rs?plain=1#L44-L66

https://github.com/hyunsik/bytesize/blob/5de80fbac0648df8d1bb10fa098f1fad45ef358c/src/lib.rs?plain=1#L73-L111

https://github.com/hyunsik/bytesize/blob/5de80fbac0648df8d1bb10fa098f1fad45ef358c/src/lib.rs?plain=1#L119-L172

The LN_KIB/LN_KB turned out not being accurate enough that some tests just failed because of the accuracy. So moving away from logarithms and exponents (like in https://github.com/hyunsik/bytesize/pull/35) will be a better choice, IMO.

There are units bigger than PB/PiB, so adding them for completeness would be great.

For tests, it's kinda unfortunate that you can't do a simple wrapper that won't affect the place where the panic is happened (at least I don't know of that one, probably can edit the built-in macros). So when one of the tests that call

https://github.com/hyunsik/bytesize/blob/5de80fbac0648df8d1bb10fa098f1fad45ef358c/src/lib.rs?plain=1#L449-L451

fails, it will show the line where the assert_eq!() is called (inside assert_to_string), and not where the test is defined. This (can) significantly slows down the debugging process.

In conclusion, there are various things that need improvement and I can offer help in rewriting some of the stuff in the code base, but considering I don't have enough free time right now, the offer is for the future.

[robjtede]

hey @Andrew15-5, sorry 😬 i made a mess trying to omit the readme changes from your PR, carrying on and collating the work from various contributions here: #69

i cherry picked your commits so you'll get attribution for this fix, thanks for your efforts :)

[Andrew15-5]

You're welcome!