Fixed inverted SI conditions (and related things) #38
Conversation
- 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.
Additional small fixes: - Removed comment about fixed bug in `./src/lib.rs`. - Rust code now has consistent 4 spaces indents in README.md.
|
@Andrew15-5 is there a reason this hasn't been merged in yet? Is this project unmaintained? |
|
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: #35 (comment). |
|
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 #35 and #25 (comment) 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: ExamplesThe 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 fails, it will show the line where the 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. |
| let unit = if si_prefix { KB } else { KIB }; | ||
| let unit_base = if si_prefix { LN_KB } else { LN_KIB }; |
There was a problem hiding this comment.
I think you're inverting it wrong? I'm confused
There was a problem hiding this comment.
SI = kB (KB), IEC = KiB (KIB).
There was a problem hiding this comment.
Ah you're right - I thought SI was IEC. Nice
There was a problem hiding this comment.
There was a problem hiding this comment.
Please make sure to use Co-authored-by if you do so. Also, I think using concrete values instead of computed constants isn't very robust, so I think you should check out #35. It replaces the ln stuff with a loop. I don't know if it will be highly less performant, but if it isn't, then it's a better implementation, IMO.
There was a problem hiding this comment.
@Andrew15-5 im lurking here and trying to understand the reasoning. you just want co-authored by so you get credit? Or is it so people can know who to talk to when they see git blame
There was a problem hiding this comment.
The first one, but I guess the second one is the outcome of the first one.
si_prefix == trueit would use"iB"instead of"B".LN_KIB&LN_KBhad swapped values../src/lib.rssince there are no bugs in the test case:// a bug case: https://github.com/flang-project/bytesize/issues/8Fixes #25
Fixes #26
Fixed #37