Number formatting: simplify and speed up toFixed()

[leeoniya]

part 3 of #34947

this is a simplified implementation of toFixed(), that is 50% faster in external/isolated testing. it does not seem to have much of an impact on the profile, though :(

there are still two tests failing (below), and maybe another case for which there is no test but was a comment about 2.25 and 2.5 needing more decimal places.

    ${'prefix:b'}         | ${undefined} | ${1532.82}                                  | ${'b1533'}
    ${'suffix:d'}         | ${undefined} | ${1532.82}                                  | ${'1533 d'}

the logic for these two tests differs from the others. the rest follow .toPrecision(3) when decimals is undefined, however in these cases it seems to expect rounding. not sure why the expectation is different here, and exactly what the logic needs to be and if it's worth having more complexity just to handle these cases.

not sure if we want this, but it's a pretty good simplification, if nothing else. 🤷‍♂️

[leeoniya]

i also found this to be rather strange:

    ${'short'}            | ${undefined} | ${1000000}                                  | ${'1 Mil'}
    ${'short'}            | ${undefined} | ${1000120}                                  | ${'1.00 Mil'}

exact values are shown with 0 decimals, but approximate values are shown with 2? 🤔

[torkelo]

Hard to review, seems to replace the old auto decimals math with toPrecision? how does that work? I cannot understand the code some comments that explain it would be nice (granted the old code & math was also cryptic ) .

[torkelo]

exact values are shown with 0 decimals, but approximate values are shown with 2?

Probably a side effect of the math that tries give a indirect measure of significant digits

[torkelo]

1532.82 with no decimals should that not be 1533 feels more correct than 1530, the other would be 1532 but why 1530 ?

[torkelo]

so this tests if the result ends with a 0 and if parsing the string back to number equals the same value?

Can we write
valStr = '' + value;
as
valStr = String(value);

Makes it a lot easier to understand that there is no string concatenation or math here

[leeoniya]

so this tests if the result ends with a 0 and if parsing the string back to number equals the same value?

yep.

i'll test, implicit coercion used to be faster than explicit, so i tend to use '' + num for toString and +str for toNum.

[torkelo]

think understand this better now :)

[leeoniya]

1532.82 with no decimals should that not be 1533 feels more correct than 1530, the other would be 1532 but why 1530 ?

yes, i agree. but i'm not sure what conditions we need to use to check for this case vs the others...and if that check will be cheap.

is it sufficient to do a magnitude check via Math.floor(Math.log10(Math.abs(value)))? can you suggest?

though one can argue that "undefined" does not mean "no decimals", 0 means no decimals. that's how it works in all other cases:

${'none'}             | ${undefined} | ${3.23}                                     | ${'3.23'}
${'none'}             | ${undefined} | ${0.0245}                                   | ${'0.0245'}
${'none'}             | ${undefined} | ${1 / 3}                                    | ${'0.333'}
${'short'}            | ${undefined} | ${1200}                                     | ${'1.20 K'}
${'short'}            | ${undefined} | ${1250}                                     | ${'1.25 K'}
${'short'}            | ${undefined} | ${1500000}                                  | ${'1.50 Mil'}
${'short'}            | ${undefined} | ${1000120}                                  | ${'1.00 Mil'}
${'short'}            | ${undefined} | ${98765}                                    | ${'98.8 K'}
${'short'}            | ${undefined} | ${9876543}                                  | ${'9.88 Mil'}
${'short'}            | ${undefined} | ${9876543}                                  | ${'9.88 Mil'}
${'kbytes'}           | ${undefined} | ${10000000}                                 | ${'9.54 GiB'}

[torkelo]

is it sufficient to do a magnitude check via Math.floor(Math.log10(Math.abs(value)))

don't know, the existing auto decimals logic is the result of a lot of trial an error and test cases :)

[dprokop]

I like the simplification of the code a lot. Slightly afraid of the pottential, small regressions/changes that this may or may not introduce.

[michaelpiron]

I wanted to raise a ticket with the request of having the possibility to define the number of significant figures instead of the number of decimals in Grafana.
While looking for existing requests/tickets, I came onto this issue.

Rounding to significant figures is a more general-purpose technique than rounding to n digits, since it handles numbers of different scales in a uniform way. But I don't find any possibility in Grafana to configure this.

Does this issue treat the same need? Or should I open a separate issue?
Thanks.

[leeoniya]

Does this issue treat the same need? Or should I open a separate issue?

this PR tries to match existing behavior, but i think there is much that can be improved. i've written some thoughts in #35838. we definitely don't need another issue, as there are already plenty open about this, all of which i've tried to find and link in the above issue.

[grafanabot]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[grafanabot]

This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!