From bdb5bedcc3dbdcfb96667d9e7a3288eabfd7bf8b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 13:07:28 -0600 Subject: [PATCH 1/6] failing test catches the bug --- libs/util/math.spec.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/libs/util/math.spec.ts b/libs/util/math.spec.ts index cca1f9b865..b76b45b1e8 100644 --- a/libs/util/math.spec.ts +++ b/libs/util/math.spec.ts @@ -26,11 +26,19 @@ it('rounds properly', () => { it.each([ [1.23, ['1', '.23']], [1, ['1', '']], // whole number decimal should be an empty string + + // values just below whole numbers + [5 - Number.EPSILON, ['5', '']], + [4.997, ['5', '']], + + // values just above whole numbers + [49.00000001, ['49', '']], + [5 + Number.EPSILON, ['5', '']], + [1.252525, ['1', '.25']], [1.259, ['1', '.26']], // should correctly round the decimal [-50.2, ['-50', '.2']], // should correctly not round down to -51 [1000.5, ['1,000', '.5']], // testing localeString - [49.00000001, ['49', '']], -])('splitDecimal', (input, output) => { +])('splitDecimal %d', (input, output) => { expect(splitDecimal(input)).toEqual(output) }) From b030d3965b3d9d1a7745561ce596e733271233ed Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 13:21:13 -0600 Subject: [PATCH 2/6] fix decimal split math --- app/components/CapacityBar.tsx | 4 ++-- app/components/CapacityMetric.tsx | 6 ++---- libs/util/math.spec.ts | 3 ++- libs/util/math.ts | 10 +++------- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/app/components/CapacityBar.tsx b/app/components/CapacityBar.tsx index 41f8503e2b..060edd9179 100644 --- a/app/components/CapacityBar.tsx +++ b/app/components/CapacityBar.tsx @@ -43,8 +43,8 @@ export const CapacityBar = ({ ({unit})
-
{wholeNumber.toLocaleString()}
-
{decimal || ''}%
+
{wholeNumber}
+
{decimal}%
diff --git a/app/components/CapacityMetric.tsx b/app/components/CapacityMetric.tsx index f26f055c83..9317c708f3 100644 --- a/app/components/CapacityMetric.tsx +++ b/app/components/CapacityMetric.tsx @@ -66,10 +66,8 @@ export const CapacityMetric = ({
- {wholeNumber.toLocaleString()} - - {decimal || ''}% - + {wholeNumber} + {decimal}%
diff --git a/libs/util/math.spec.ts b/libs/util/math.spec.ts index b76b45b1e8..8eabfde352 100644 --- a/libs/util/math.spec.ts +++ b/libs/util/math.spec.ts @@ -19,6 +19,7 @@ it('rounds properly', () => { expect(round(123.0001, 3)).toEqual(123) // period is culled if decimals are all zeros expect(round(1.9, 0)).toEqual(2) expect(round(1.9, 1)).toEqual(1.9) + expect(round(4.997, 2)).toEqual(5) expect(round(5 / 2, 2)).toEqual(2.5) // math expressions are resolved expect(round(1879048192 / GiB, 2)).toEqual(1.75) // constants can be evaluated }) @@ -39,6 +40,6 @@ it.each([ [1.259, ['1', '.26']], // should correctly round the decimal [-50.2, ['-50', '.2']], // should correctly not round down to -51 [1000.5, ['1,000', '.5']], // testing localeString -])('splitDecimal %d', (input, output) => { +])('splitDecimal %d -> %s', (input, output) => { expect(splitDecimal(input)).toEqual(output) }) diff --git a/libs/util/math.ts b/libs/util/math.ts index 4b36a4b2e9..a683e22a2f 100644 --- a/libs/util/math.ts +++ b/libs/util/math.ts @@ -6,13 +6,9 @@ * Copyright Oxide Computer Company */ -export function splitDecimal(value: number) { - const wholeNumber = Math.trunc(value) - const decimal = value % 1 !== 0 ? round(value % 1, 2) : null - return [ - wholeNumber.toLocaleString(), - decimal ? '.' + decimal.toLocaleString().split('.')[1] : '', - ] +export function splitDecimal(value: number): [string, string] { + const [whole, decimal] = round(value, 2).toLocaleString().split('.') + return [whole, decimal ? '.' + decimal : ''] } export function round(num: number, digits: number) { From 7df7964969598cc60fc666f47ef89d4a0525316a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 13:41:21 -0600 Subject: [PATCH 3/6] delete unused CapacityMetric component --- app/components/CapacityMetric.tsx | 87 ------------------------------- 1 file changed, 87 deletions(-) delete mode 100644 app/components/CapacityMetric.tsx diff --git a/app/components/CapacityMetric.tsx b/app/components/CapacityMetric.tsx deleted file mode 100644 index 9317c708f3..0000000000 --- a/app/components/CapacityMetric.tsx +++ /dev/null @@ -1,87 +0,0 @@ -/* - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * Copyright Oxide Computer Company - */ -import { useMemo } from 'react' - -import { useApiQuery, type SystemMetricName } from '@oxide/api' -import { splitDecimal } from '@oxide/util' - -import RoundedSector from 'app/components/RoundedSector' - -// exported to use in the loader because it needs to be identical -export const capacityQueryParams = { - // beginning of time, aka 1970 - startTime: new Date(0), - // kind of janky to use pageload time. we can think about making it live - // later. ideally refetch would be coordinated with the graphs - endTime: new Date(), - limit: 1, - order: 'descending' as const, -} - -export const CapacityMetric = ({ - icon, - title, - // unit, - metricName, - valueTransform = (x) => x, - capacity, -}: { - icon: JSX.Element - title: string - // unit: string - metricName: SystemMetricName - valueTransform?: (n: number) => number - capacity: number -}) => { - // this is going to return at most one data point - const { data } = useApiQuery( - 'systemMetric', - { path: { metricName }, query: capacityQueryParams }, - { placeholderData: (x) => x } - ) - - const metrics = useMemo(() => data?.items || [], [data]) - const datum = metrics && metrics.length > 0 ? metrics[metrics.length - 1].datum.datum : 0 - // it's always a number but let's rule out the other options without doing a cast - const utilization = valueTransform(typeof datum === 'number' ? datum : 0) - const utilizationPct = (utilization * 100) / capacity - const [wholeNumber, decimal] = splitDecimal(utilizationPct) - - return ( -
-
-
- {title} - {/* ({unit}) */} -
- -
-
- {icon} -
- -
- {wholeNumber} - {decimal}% -
-
-
- -
-
- -
-
-
- ) -} From e1c48685a1a24c9b8e49b0e2e23d80f9f9f8a579 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 14:08:30 -0600 Subject: [PATCH 4/6] use Intl.NumberFormat to incredible effect --- libs/util/array.spec.tsx | 9 ++++- libs/util/array.ts | 8 +++++ libs/util/math.spec.ts | 74 +++++++++++++++++++++++++++++++--------- libs/util/math.ts | 22 ++++++++++-- 4 files changed, 94 insertions(+), 19 deletions(-) diff --git a/libs/util/array.spec.tsx b/libs/util/array.spec.tsx index d948eccf54..cf6226a6e9 100644 --- a/libs/util/array.spec.tsx +++ b/libs/util/array.spec.tsx @@ -8,7 +8,7 @@ import { type ReactElement } from 'react' import { expect, test } from 'vitest' -import { groupBy, intersperse, lowestBy, sortBy, sumBy } from './array' +import { groupBy, intersperse, lowestBy, sortBy, splitOnceBy, sumBy } from './array' test('sortBy', () => { expect(sortBy(['d', 'b', 'c', 'a'])).toEqual(['a', 'b', 'c', 'd']) @@ -97,3 +97,10 @@ test('intersperse', () => { expect(result.map(getText)).toEqual(['a', ',', 'b', ',', 'or', 'c']) expect(result.map(getKey)).toEqual(['a', 'sep-1', 'b', 'sep-2', 'conj', 'c']) }) + +test('splitOnceBy', () => { + expect(splitOnceBy([], () => false)).toEqual([[], []]) + expect(splitOnceBy([1, 2, 3], () => false)).toEqual([[1, 2, 3], []]) + expect(splitOnceBy([1, 2, 3], () => true)).toEqual([[], [1, 2, 3]]) + expect(splitOnceBy([1, 2, 3], (x) => x % 2 === 0)).toEqual([[1], [2, 3]]) +}) diff --git a/libs/util/array.ts b/libs/util/array.ts index ab5ff348b4..bc2de39634 100644 --- a/libs/util/array.ts +++ b/libs/util/array.ts @@ -102,3 +102,11 @@ export function intersperse( return [sep0, item] }) } + +/** + * Split array at first element where `by` is true. That element lands in the second array. + */ +export function splitOnceBy(array: T[], by: (t: T) => boolean) { + const i = array.findIndex(by) + return i === -1 ? [array, []] : [array.slice(0, i), array.slice(i)] +} diff --git a/libs/util/math.spec.ts b/libs/util/math.spec.ts index 8eabfde352..9f8a717b27 100644 --- a/libs/util/math.spec.ts +++ b/libs/util/math.spec.ts @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, it } from 'vitest' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { GiB } from '.' import { round, splitDecimal } from './math' @@ -24,22 +24,64 @@ it('rounds properly', () => { expect(round(1879048192 / GiB, 2)).toEqual(1.75) // constants can be evaluated }) -it.each([ - [1.23, ['1', '.23']], - [1, ['1', '']], // whole number decimal should be an empty string +describe('splitDecimal', () => { + describe('with default locale', () => { + it.each([ + [1.23, ['1', '.23']], + [1, ['1', '']], // whole number decimal should be an empty string - // values just below whole numbers - [5 - Number.EPSILON, ['5', '']], - [4.997, ['5', '']], + // values just below whole numbers + [5 - Number.EPSILON, ['5', '']], + [4.997, ['5', '']], - // values just above whole numbers - [49.00000001, ['49', '']], - [5 + Number.EPSILON, ['5', '']], + // values just above whole numbers + [49.00000001, ['49', '']], + [5 + Number.EPSILON, ['5', '']], - [1.252525, ['1', '.25']], - [1.259, ['1', '.26']], // should correctly round the decimal - [-50.2, ['-50', '.2']], // should correctly not round down to -51 - [1000.5, ['1,000', '.5']], // testing localeString -])('splitDecimal %d -> %s', (input, output) => { - expect(splitDecimal(input)).toEqual(output) + [1.252525, ['1', '.25']], + [1.259, ['1', '.26']], // should correctly round the decimal + [-50.2, ['-50', '.2']], // should correctly not round down to -51 + [1000.5, ['1,000', '.5']], // test localeString grouping + ])('splitDecimal %d -> %s', (input, output) => { + expect(splitDecimal(input)).toEqual(output) + }) + }) + + describe('with de-DE locale', () => { + const originalLanguage = global.navigator.language + + beforeAll(() => { + Object.defineProperty(global.navigator, 'language', { + value: 'de-DE', + writable: true, + }) + }) + + it.each([ + [1.23, ['1', ',23']], + [1, ['1', '']], // whole number decimal should be an empty string + + // values just below whole numbers + [5 - Number.EPSILON, ['5', '']], + [4.997, ['5', '']], + + // values just above whole numbers + [49.00000001, ['49', '']], + [5 + Number.EPSILON, ['5', '']], + + [1.252525, ['1', ',25']], + [1.259, ['1', ',26']], // should correctly round the decimal + [-50.2, ['-50', ',2']], // should correctly not round down to -51 + [1000.5, ['1.000', ',5']], // test localeString grouping + ])('splitDecimal %d -> %s', (input, output) => { + expect(splitDecimal(input)).toEqual(output) + }) + + afterAll(() => { + Object.defineProperty(global.navigator, 'language', { + value: originalLanguage, + writable: true, + }) + }) + }) }) diff --git a/libs/util/math.ts b/libs/util/math.ts index a683e22a2f..ee4eda7a71 100644 --- a/libs/util/math.ts +++ b/libs/util/math.ts @@ -6,11 +6,29 @@ * Copyright Oxide Computer Company */ +import { splitOnceBy } from '.' + +/** + * Get the two parts of a number (before decimal and after-and-including + * decimal) as strings. Round to 2 decimal points if necessary. + * + * If there is no decimal, we will only have whole parts (which can include minus + * sign, group separators [comma in en-US], and of course actual number + * groups). Those will get joined and the decimal part will be th eempty string. + */ export function splitDecimal(value: number): [string, string] { - const [whole, decimal] = round(value, 2).toLocaleString().split('.') - return [whole, decimal ? '.' + decimal : ''] + const nf = Intl.NumberFormat(navigator.language, { maximumFractionDigits: 2 }) + const parts = nf.formatToParts(value) + + const [wholeParts, decimalParts] = splitOnceBy(parts, (p) => p.type === 'decimal') + + return [ + wholeParts.map((p) => p.value).join(''), + decimalParts.map((p) => p.value).join(''), + ] } +// TODO: convert to use Intl.NumberFormat.format() export function round(num: number, digits: number) { return Number(num.toFixed(digits)) } From 9ff9b9fb0d13724836c501d5db25788052903418 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 14:15:52 -0600 Subject: [PATCH 5/6] might as well use Intl.NumberFormat for round() too --- libs/util/math.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/util/math.ts b/libs/util/math.ts index ee4eda7a71..0af7c57b17 100644 --- a/libs/util/math.ts +++ b/libs/util/math.ts @@ -12,9 +12,9 @@ import { splitOnceBy } from '.' * Get the two parts of a number (before decimal and after-and-including * decimal) as strings. Round to 2 decimal points if necessary. * - * If there is no decimal, we will only have whole parts (which can include minus - * sign, group separators [comma in en-US], and of course actual number - * groups). Those will get joined and the decimal part will be th eempty string. + * If there is no decimal, we will only have whole parts (which can include + * minus sign, group separators [comma in en-US], and of course actual number + * groups). Those will get joined and the decimal part will be the empty string. */ export function splitDecimal(value: number): [string, string] { const nf = Intl.NumberFormat(navigator.language, { maximumFractionDigits: 2 }) @@ -28,7 +28,7 @@ export function splitDecimal(value: number): [string, string] { ] } -// TODO: convert to use Intl.NumberFormat.format() export function round(num: number, digits: number) { - return Number(num.toFixed(digits)) + const nf = Intl.NumberFormat(navigator.language, { maximumFractionDigits: digits }) + return Number(nf.format(num)) } From a971a8c1ea20dd10804bf9f6393b13ae89d21705 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 14 Feb 2024 14:22:14 -0600 Subject: [PATCH 6/6] make sure we're handling numbers between -1 and 1 --- libs/util/math.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/libs/util/math.spec.ts b/libs/util/math.spec.ts index 9f8a717b27..5d5f743d4f 100644 --- a/libs/util/math.spec.ts +++ b/libs/util/math.spec.ts @@ -11,6 +11,8 @@ import { GiB } from '.' import { round, splitDecimal } from './math' it('rounds properly', () => { + expect(round(0.456, 2)).toEqual(0.46) + expect(round(-0.456, 2)).toEqual(-0.46) expect(round(123.456, 0)).toEqual(123) expect(round(123.456, 1)).toEqual(123.5) expect(round(123.456, 2)).toEqual(123.46) @@ -27,12 +29,17 @@ it('rounds properly', () => { describe('splitDecimal', () => { describe('with default locale', () => { it.each([ + [0.23, ['0', '.23']], + [0.236, ['0', '.24']], + [-0.236, ['-0', '.24']], [1.23, ['1', '.23']], [1, ['1', '']], // whole number decimal should be an empty string // values just below whole numbers [5 - Number.EPSILON, ['5', '']], [4.997, ['5', '']], + [-4.997, ['-5', '']], + [0.997, ['1', '']], // values just above whole numbers [49.00000001, ['49', '']], @@ -58,12 +65,17 @@ describe('splitDecimal', () => { }) it.each([ + [0.23, ['0', ',23']], + [0.236, ['0', ',24']], + [-0.236, ['-0', ',24']], [1.23, ['1', ',23']], [1, ['1', '']], // whole number decimal should be an empty string // values just below whole numbers [5 - Number.EPSILON, ['5', '']], [4.997, ['5', '']], + [-4.997, ['-5', '']], + [0.997, ['1', '']], // values just above whole numbers [49.00000001, ['49', '']],