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.

Sign up for GitHub

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

Jump to bottom

Fixed inverted SI conditions (and related things) #38

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

Fixed inverted SI conditions (and related things) #38

wants to merge 4 commits into from

Conversation

Andrew15-5
Copy link

@Andrew15-5 Andrew15-5 commented Oct 6, 2023 •
edited
Loading

  • 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

@Andrew15-5
Fixed inverted SI conditions
04c4c29 
- 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.
@Andrew15-5
Swaped (fixed) values of LN_KIB & LN_KB
1ccab13
@Andrew15-5
Updated tests in README.md
4d7c8d9 
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 Andrew15-5 mentioned this pull request Oct 6, 2023
Open
@andrewgazelka
Copy link

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

@Andrew15-5
Copy link
Author

Andrew15-5 commented Apr 30, 2024 •
edited
Loading

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).

@Andrew15-5
Copy link
Author

Andrew15-5 commented Nov 18, 2024 •
edited
Loading

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:

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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display for IEC units LN_KIB / LN_KB are backwards SI unit should be decimal units
2 participants
@Andrew15-5 @andrewgazelka