bmuraykhi commented on PR #42437:
URL: https://github.com/apache/superset/pull/42437#issuecomment-5083640692
Both review findings are addressed.
**Rounding at unit boundaries** (07e98d2551). The suffix index was taken
from the raw value *before* rounding, so anything that rounded up to the base
kept the previous unit:
| input | before | expected |
|---|---|---|
| `999999` | `1000kbps` | `1Mbps` |
| `999999999` | `1000Mbps` | `1Gbps` |
| `999.999` | `1000bps` | `1kbps` |
| `999.6` (`decimals: 0`) | `1000bps` | `1kbps` |
| `124999.99` (`fromBytes`) | `1000kbps` | `1Mbps` |
The fix re-evaluates the index after rounding, guarded with `i <
SUFFIXES.length - 1` so the largest suffix saturates rather than overflowing —
`1000Qbps` is preserved. Every pre-existing assertion produces identical
output, plus a new test covering both signs, both conversion paths, and
`decimals: 0`.
**Missing assertions for the registrations** (3aacc27e92).
`setupFormatters.test.ts` now asserts both `NETWORK_THROUGHPUT` and
`NETWORK_THROUGHPUT_FROM_BYTES` are registered against the formatter instance
returned by the matching `createThroughputFormatter` call, so a dropped,
renamed, or swapped registration fails.
**One pre-existing bug worth flagging.** While confirming the rounding fix I
found that `createMemoryFormatter` has the same bug, plus a second one at the
bottom of the scale:
```js
createMemoryFormatter()(999999) // '1000kB' — expected '1MB'
createMemoryFormatter()(0.5) // '500undefined' — index reaches -1, so
the value is
// divided by 1000⁻¹ and suffixes[-1] is
undefined
```
`createThroughputFormatter` guards against both — the index is clamped with
`Math.max(0, …)` and re-evaluated after rounding — which is why the new code
looks slightly different from its neighbour. I've left `createMemoryFormatter`
untouched to keep this PR scoped to the new formats, and will open a separate
`fix:` PR for it. Happy to fold it in here instead if you'd prefer it as one
change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
