innovark37 commented on code in PR #39330:
URL: https://github.com/apache/superset/pull/39330#discussion_r3259730879
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createDurationFormatter.ts:
##########
@@ -17,23 +17,58 @@
* under the License.
*/
-import prettyMilliseconds, { Options } from 'pretty-ms';
import NumberFormatter from '../NumberFormatter';
+import { getIntlDurationFormatter } from '../utils/getIntlDurationFormatter';
+import { parseMilliseconds } from '../utils/parseMilliseconds';
export default function createDurationFormatter(
config: {
description?: string;
id?: string;
label?: string;
multiplier?: number;
- } & Options = {},
+ locale?: string;
+ formatSubMilliseconds?: boolean;
+ } & Intl.DurationFormatOptions = {},
) {
- const { description, id, label, multiplier = 1, ...prettyMsOptions } =
config;
-
+ const {
+ description,
+ id,
+ label,
+ multiplier = 1,
+ locale,
+ formatSubMilliseconds = false,
+ ...intlOptions
+ } = config;
+ const durationFormatter = getIntlDurationFormatter(locale, {
+ secondsDisplay: 'auto',
+ style: 'narrow',
+ ...intlOptions,
+ });
+ const zeroDurationFormatter = getIntlDurationFormatter(locale, {
+ secondsDisplay: 'always',
+ style: 'narrow',
+ ...intlOptions,
+ });
return new NumberFormatter({
description,
- formatFunc: value =>
- prettyMilliseconds(value * multiplier, prettyMsOptions),
+ formatFunc: value => {
+ const durObject = parseMilliseconds(value * multiplier);
+
+ if (!formatSubMilliseconds) {
+ durObject.milliseconds = 0;
Review Comment:
This behavior is intentional. The goal of this change is not to exactly
emulate `pretty-ms` output, but to move duration formatting to
`Intl.DurationFormat` so it follows the localized Intl model.
By default we omit sub-second fields, so values like 10500ms are formatted
as 10s. Callers that need sub-second precision can opt in with
formatSubMilliseconds: true, which preserves
milliseconds/microseconds/nanoseconds and gives output like 100ms 400μs 80ns
depending on the options.
I agree this is a visible behavior change from pretty-ms, so I can call it
out explicitly if needed, but I would prefer to keep the
Intl.DurationFormat-based default.
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createDurationFormatter.ts:
##########
@@ -17,23 +17,58 @@
* under the License.
*/
-import prettyMilliseconds, { Options } from 'pretty-ms';
import NumberFormatter from '../NumberFormatter';
+import { getIntlDurationFormatter } from '../utils/getIntlDurationFormatter';
+import { parseMilliseconds } from '../utils/parseMilliseconds';
export default function createDurationFormatter(
config: {
description?: string;
id?: string;
label?: string;
multiplier?: number;
- } & Options = {},
+ locale?: string;
+ formatSubMilliseconds?: boolean;
+ } & Intl.DurationFormatOptions = {},
) {
- const { description, id, label, multiplier = 1, ...prettyMsOptions } =
config;
-
+ const {
+ description,
+ id,
+ label,
+ multiplier = 1,
+ locale,
+ formatSubMilliseconds = false,
+ ...intlOptions
+ } = config;
+ const durationFormatter = getIntlDurationFormatter(locale, {
+ secondsDisplay: 'auto',
+ style: 'narrow',
+ ...intlOptions,
+ });
+ const zeroDurationFormatter = getIntlDurationFormatter(locale, {
+ secondsDisplay: 'always',
+ style: 'narrow',
+ ...intlOptions,
+ });
return new NumberFormatter({
description,
- formatFunc: value =>
- prettyMilliseconds(value * multiplier, prettyMsOptions),
+ formatFunc: value => {
+ const durObject = parseMilliseconds(value * multiplier);
+
+ if (!formatSubMilliseconds) {
+ durObject.milliseconds = 0;
+ durObject.microseconds = 0;
+ durObject.nanoseconds = 0;
+ }
+
+ const isAllUnitsZero = Object.values(durObject).every(
+ value => value === 0,
+ );
+
+ return (
+ isAllUnitsZero ? zeroDurationFormatter : durationFormatter
+ ).format(durObject);
+ },
Review Comment:
I double-checked the current parse-ms implementation, and it does not use
`Math.abs()`; it preserves the sign via `Math.trunc()` for each duration field.
I also already have `createDurationFormatter` tests covering negative values
such as `-1000ms` and `-0.5s`.
That said, this uncovered a related edge case in our wrapper: the
days-to-years conversion used `Math.floor()`, which can incorrectly overflow
negative durations into years (for example, -364 days). I changed that to
`Math.trunc()` and added a regression test for negative day durations.
--
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]
