oxidecomputer / oxidecomputer/console

Separate unit conversion from rounding

Open
#2,015 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
228
Forks
22
Avg merge
19h 42m
Merged PRs (30d)
32

Description

Short version:

  1. Make round return a string
    a. Possibly make useGrouping configurable
  2. Don't round in bytesToGiB and bytesToGiB
  3. Go through all the places where those are used (non trivial because in plenty of spots we call the function and then pass the result down as a prop elsewhere) and make sure we're calling round at display time as appropriate

I'm only writing this all out because I got this far but don't feel like fixing it completely today.


As @benjaminleonard points out in https://github.com/oxidecomputer/console/pull/2011#discussion_r1507408890, rounding is really about displaying numbers on screen. If you're going to do arithmetic, there's almost never a reason to round first. So it doesn't really make sense that round returns a number, and returning a string would have advantages, like

  1. letting us useGrouping, i.e., put the lovely group-separating commas in there when we want, and
  2. making sure we're not doing arithmetic on it afterward.

So great, let's return a string:

-export function round(num: number, digits: number) {
-  // unlike with splitDecimal, we hard-code en-US to ensure that Number() will
-  // be able to parse the result
-  const nf = Intl.NumberFormat('en-US', {
-    maximumFractionDigits: digits,
-    // very important, otherwise turning back into number will fail on n >= 1000
-    // due to commas
-    useGrouping: false,
-  })
-  return Number(nf.format(num))
-}
+export const round = (num: number, digits: number) =>
+  Intl.NumberFormat(navigator.language, { maximumFractionDigits: digits }).format(num)

The problem is that we have (so far) coupled our GiB and TiB conversion stuff to round because we use it for display:

https://github.com/oxidecomputer/console/blob/25fb506d0527f5508dc90b118672c0bf8ce14dd3/app/util/units.ts#L15-L16

but there are also spots where we take a bytesToGiB output and do further math on it:

https://github.com/oxidecomputer/console/blob/25fb506d0527f5508dc90b118672c0bf8ce14dd3/app/api/util.ts#L139

https://github.com/oxidecomputer/console/blob/25fb506d0527f5508dc90b118672c0bf8ce14dd3/app/forms/disk-create.tsx#L123

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in app/util/units.ts and inspect the bytesToGiB and related conversion helpers, then trace their uses in app/api/util.ts and app/forms/disk-create.tsx. Review all callers to distinguish arithmetic from display, and confirm that conversions remain unrounded while rounding occurs at display time.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.