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. |
Signed-off-by: MrCroxx <[email protected]>
* Fix parsing logic after first whitespace The way the parse was implemented accepted additional numeric characters or `.` after the first valid `f64` literal but ignored them. This permitted more strings that are invalid following a strict grammar for byte sizes and silently ignores the further symbols without error. ``` 1.0 ...KB 1.0 42.0 B ``` This change makes those illegal. `1 000 B` was also subject to the bad `skip_while` ignoring the following `000`. In this version of the fix whitespace is not accepted as a digit separator. So it will raise an error if the user doesn't explicitly strip the whitespace instead of reporting a wrong value. * Add tests for invalid chars after whitespace * Add test documenting that whitespace is not a valid digit separator More following the old choices of using `f64::from_str` --------- Co-authored-by: Rob Ede <[email protected]>
|
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 :) |
|
You're welcome! |
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