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.

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

fix: implementation of file size formatting #379

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

Antosser
Copy link
Contributor

@Antosser Antosser commented Nov 4, 2023

This PR removes src/utils/fmt.rs, since it only had the format_bytes function, which can also be used from another library.

The implementation of format_bytes is also incorrect. 1024 bytes is not equal to 1KB, as the test indicates. 1024 bytes is equal to 1KiB, and means Kibibyte, where bi stands for binary. Why not just use a library for that? That would eliminate such bugs.

The PR changes the file size type from String to u64 and adds a _bytes postfix to the property name.

- pub(crate) size: String,
+ pub(crate) size_bytes: u64,

I would love to store the file size as the Size struct provided by the size crate and remove the postfix, but Size doesn't implement the Serialize trait, and we'd have to wait for that before we can do that.

@Antosser Antosser changed the title Fix: implementation of filesize formatting Fix: implementation of file size formatting Nov 4, 2023
@EstebanBorai
Copy link
Member

Usually computers doesnt render size using Base-2, the use of kilo, giga, and mega is
more of jargon than actually the value.

Please take a peek on these two pictures:

This is the current main:

current

This is this branch proposal:

branch

My observations are:

  • macOS Get Info window uses Kilobytes instead of Kibibytes
  • Both size values (skip unit please) are the same value, but in neither of both the size matches OS specific size. (Float point precision is a well known issue).

@EstebanBorai EstebanBorai changed the title Fix: implementation of file size formatting fix: implementation of file size formatting Nov 4, 2023
Co-authored-by: Esteban Borai <estebanborai@gmail.com>
@Antosser
Copy link
Contributor Author

Antosser commented Nov 4, 2023

Usually computers doesnt render size using Base-2, the use of kilo, giga, and mega is
more of jargon than actually the value.

Windows uses binary but incorrectly displays KiB as KB

@Antosser
Copy link
Contributor Author

Antosser commented Nov 4, 2023

The point is, the way it is right now is incorrect. The number is binary while the unit is decimal

@Antosser
Copy link
Contributor Author

Antosser commented Nov 4, 2023

but in neither of both the size matches OS specific size

That's because the number in http-server is in KiB, while the OS size is in bytes. It has almost nothing to do with floats

This reverts commit ad300bb.
@EstebanBorai
Copy link
Member

Usually computers doesnt render size using Base-2, the use of kilo, giga, and mega is

more of jargon than actually the value.

Windows uses binary but incorrectly displays KiB as KB

Im struggling finding an OS that uses Base-2 notations in their data size notations.

Even though I consider you are technically right, I still have to consider the user base counter part, which not only involves the technical side of the problem, but the educational.

OS developers and the Software Industry have been using Base-10 notation for years.

This would introduce confusion, and also would affect current users perception on the data size values because HTTP Server would render a different unit than their systems.

@EstebanBorai
Copy link
Member

The point is, the way it is right now is incorrect. The number is binary while the unit is decimal

Thing is, no popular system uses binary notation. Its a matter of usage and how the data industry popularized data units.

Using Base-2 would be against that, please take into consideration all kind of users.

@EstebanBorai
Copy link
Member

but in neither of both the size matches OS specific size

That's because the number in http-server is in KiB, while the OS size is in bytes. It has almost nothing to do with floats

Please pay attention to the size value in macOS Get Info window and compare it to HTTP Server's size value.

@Antosser
Copy link
Contributor Author

Antosser commented Nov 5, 2023

This commit now uses the humansize crate, which allows choosing BINARY vs. DECIMAL. This is what if now looks like:
image

@Antosser
Copy link
Contributor Author

Antosser commented Nov 5, 2023

These are the possible options provided by the crate:
image

@EstebanBorai EstebanBorai merged commit 5f8796e into http-server-rs:main Nov 15, 2023
7 checks passed
@Antosser Antosser deleted the correctfilesize branch November 15, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants