Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Refactor number-formatter to use integers #1608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ip1981
Copy link
Member

@ip1981 ip1981 commented Feb 25, 2020

Make it print precise numbers if they are small enough.

number-formatter

Closes #1567.


This change is Reviewable

@ip1981 ip1981 requested a review from iml-team February 25, 2020 15:02
@ip1981 ip1981 self-assigned this Feb 25, 2020
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @iml-team and @ip1981)


iml-gui/crate/src/page/filesystem.rs, line 172 at r1 (raw file):

fn files_view<I>(fs: &Filesystem) -> Node<I> {
    if let Some((u, t)) = fs.files_total.and_then(|t| fs.files_free.map(|f| (t - f, t))) {
        log!("used: {}, total: {}", u, t);

Why did this log disappear?


number-formatter/src/lib.rs, line 53 at r1 (raw file):

        assert_eq!(format_bytes(5_323_330_102_372, Some(2)), "4.84 TiB");
        assert_eq!(format_bytes(84_567_942_345_572_238, Some(2)), "75.11 PiB");
    }

ZiB test case is missing in new version.

Turns out it's because it would overflow u64.

Isn't it misleading to leave Z and Y suffices in the array then?

Also, isn't it reduction of functionality that might be undesirable?


number-formatter/src/lib.rs, line 57 at r1 (raw file):

    #[test]
    fn test_format_number() {
        // TODO: assert_eq!(format_number(999999, Some(0)), "1M");

A TODO

@ip1981
Copy link
Member Author

ip1981 commented Feb 29, 2020

  1. The log disappeared because it shouldn't had sneaked in :)
  2. AFAIK we are already limited to u64 in other places, and f64 could not be used to represent the exact number > max of u64, so the test didn't make much sense anyway (I think). We might use u128, but to my knowledge u64 is a lot and still enough.
  3. As for the TODO test, it is complicated. We might just ignore that, or stop using from logarithms and float number altogether. I put the test here as as reminder.

Reviewable is down at the time of writing :)

@ip1981
Copy link
Member Author

ip1981 commented Feb 29, 2020

For what I've seen in the API part, we are using Django's IntegerField which is i32. It becomes f64 because JS only supports f64 (and max integer 2^53 - 1).

@jgrund
Copy link
Member

jgrund commented Feb 29, 2020

For what I've seen in the API part, we are using Django's IntegerField which is i32. It becomes f64 because JS only supports f64 (and max integer 2^53 - 1).

Keep in mind that within the next week we will be using influx to get these numbers.

@jgrund
Copy link
Member

jgrund commented Feb 29, 2020

because JS only supports f64 (and max integer 2^53 - 1).

We are not restricted to JS here, but Rust instead since this ultimately is compiled to WebAssembly.

@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from e1fc40d to 75fc3de Compare March 9, 2020 15:41
@ip1981
Copy link
Member Author

ip1981 commented Mar 9, 2020

Keep in mind that within the next week we will be using influx to get these numbers.

Still u64 is two times more than enough (i64) :)

@ip1981
Copy link
Member Author

ip1981 commented Mar 9, 2020

because JS only supports f64 (and max integer 2^53 - 1).

We are not restricted to JS here, but Rust instead since this ultimately is compiled to WebAssembly.

I was trying to say that so far we were limited by JS and were not actually using large enough numbers, or somewhere there were incorrect appearance.

@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch 4 times, most recently from 7419cba to 93cd429 Compare March 18, 2020 10:39
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @iml-team and @ip1981)

@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from 93cd429 to d750325 Compare March 20, 2020 15:43
@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from d750325 to 36661aa Compare April 15, 2020 15:35
@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from 36661aa to ad848ce Compare April 27, 2020 15:41
@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from ad848ce to 2fbd246 Compare June 8, 2020 10:19
Make it print precise numbers if they are small enough.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
@ip1981 ip1981 force-pushed the ip1981/1567-numbers branch from 2fbd246 to f6837ff Compare July 1, 2020 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

number-formatter should be more precise
3 participants