Clean up and type number formatting APIs #20334

allisonkarlitskaya · 2024-04-18T10:48:17Z

move a very specific API (cockpit.get_byte_units()) with a single user (storaged) into that user (along with its unit tests), and simplify it a bit
add a new API style for our number formatting APIs that removes many of the existing warts
port test-format to TypeScript

pkg/lib/cockpit.d.ts

allisonkarlitskaya · 2024-04-18T11:03:51Z

The content of this PR is basically my best crack at #20322 (comment)

I'd like to actually break the API, given that there are zero known users; but I'm too scared about the unknown ones. So, my gut feeling:

Design the API as we want it (see my proposal above, I'm happy to discuss of course)

Do the types exactly for this, so that we learn about unintended API usage via type checks

Then add some backwards compat hacks inside of format_bytes() to opportunistically interpret a non-object second argument: if it's "1024", treat it as { binary: true }, and ignore all others. console.warn() about it in any case.

Is that possible/reasonable?

martinpitt

Thanks for figuring this out! Aside from a few implementation fixmes this goes into a good direction. I'd like to clean it up more, but that doesn't have to block this PR, it should be a follow-up.

pkg/storaged/utils.js

martinpitt · 2024-04-18T11:27:51Z

pkg/lib/cockpit.js

+            if (!is_object(options))
+                options = { separate: options };
+
+            options.factor = second_arg;


This looks unclean: If options is already an object, it is passed in from the caller. This will modify that passed-in argument. So as an intermediate step, this is safer: options = {...options, factor: second_arg} (i.e. make a copy).

But even more so this is a field which isn't documented or covered by types, so doesn't this raise some ts alarm?

WDYT about something like

if (second_arg === 1024) options = {...options, base2: true } else console.warn("format_units(): Ignoring invalid factor value", second_arg);

?

Thanks. This is a good suggestion and I'm surprised I was sloppy about this.

pkg/lib/cockpit.js

jelly · 2024-04-18T12:18:07Z

pkg/lib/cockpit.d.ts

+    function format_number(n: MaybeNumber, precision?: number): string
+    function format_bytes(n: MaybeNumber, options?: FormatOptions): string;
+    function format_bytes_per_sec(n: MaybeNumber, options?: FormatOptions): string;
+    function format_bits_per_sec(n: MaybeNumber, options?: FormatOptions & { base2?: false }): string;


Hmmm, so the argument here is. if I wanted to provide an options argument, I'd have to figure out the default factor which would be error prone?

The new API basically has two possibilities: SI units (default, old factor=1000) and IEC units (old factor=1024).

@jelly With the "old" API you could specify undefined for the factor, in which case it would be the default "1000". But indeed this is error prone -- in fact, my first version here gave null, which made it fall over completely. That's why I'd like to get rid of that.

This has a single user (in storaged) and is more general than it needs to be to support that user. Hardcode it to SI-based units and move it into storaged/utils.js, so we can keep the unit tests. The unit tests themselves only test the 1024 case (which is the opposite of what is actually used in practice) so adjust them for SI (mostly by renaming variables and changing `1024`s to `1000`s).

martinpitt · 2024-04-18T14:28:03Z

these failures are actually relevant -- first of all, a unit conversion got lost along the way (see screenshot), and now I agree that we we need to restrict (or disable) the cockpit.warning(). Sorry for the bad advice.

Thanks to Martin for some ideas there.

Add a small function called f() to qunit-tests.ts. It is intended to be used as a tagged template string format function. The default formatting for template strings passes each argument to String(), which means that objects get formatted like: [object Object] This function formats them using JSON, so you can see the actual object value. It can be imported into any QUnit test and used like: f`format_bytes_per_sec(${checks[i][0]}, ${checks[i][1]}, ${checks[i][2]})` which produces output like: ok 121 - format_bytes_per_sec: format_bytes_per_sec(25555678, "kB/s", {"precision":2})

Use our fancy new tagged templates to name our assertions in test-format. In addition to being a lot easier (and shorter) to write, it means we see `{"precision": 2}` instead of `[object Object]`. Also, leave out the value that we're comparing it equal to. This value is already displayed on assertion failures, and having it shown twice only makes the output harder to read.

allisonkarlitskaya · 2024-04-18T15:53:04Z

these failures are actually relevant -- first of all, a unit conversion got lost along the way (see screenshot)

This comes down to

export function format_fsys_usage(used, total) {
    let text = "";
    let parts = cockpit.format_bytes(total, undefined, { separate: true, precision: 2 });
    text = " / " + parts.join(" ");
    const unit = parts[1];

    parts = cockpit.format_bytes(used, unit, { separate: true, precision: 2 });
    return parts[0] + text;
}

ie: another API doing the same thing as the usage you were trying to eliminate from podman in cockpit-project/cockpit-podman#1673. cc @garrett

Let's make sure we don't regress this with changes to `cockpit.format_units()`.

Add a new simplified style for all of the units-based number formatting APIs (bytes, bytes/sec, bits/sec). Keep loose versions of the old APIs around in `cockpit.d.ts`, but mark them deprecated. Make sure we test the new API variant from test-format.js.

This was pretty much completely pain-free, with a minor exception: the test overrides the `cockpit.language` attribute, which we would otherwise probably declare as `const`.

martinpitt

Brilliant, thanks! /devel failed with an unexpected message in TestTerminal.testBasic ("truncated data in external channel"), but I think that's an unrelated flake. The weather report shows the same message in TestSuperuser.testBasic here and here.

I retry it to be sure.

@jelly can you please also check this again?

pkg/lib/qunit-tests.ts

martinpitt · 2024-04-19T05:41:05Z

pkg/lib/cockpit.d.ts

+    function format_number(n: MaybeNumber, precision?: number): string
+    function format_bytes(n: MaybeNumber, options?: FormatOptions): string;
+    function format_bytes_per_sec(n: MaybeNumber, options?: FormatOptions): string;
+    function format_bits_per_sec(n: MaybeNumber, options?: FormatOptions & { base2?: false }): string;


Just to make sure I understand this: The base2 restriction is here to make base2: true fall through the "deprecated" declaration, right? Not because enabling base2 would actually change the output type.

I'm pretty sure that enabling base2 will break this (ie: return just the number) in exactly the same way as passing 1024 as factor would. That particular function lacks base2 units (ie: no Mibps or so)...

Right, and I don't think we ever supported that actually.

jelly · 2024-04-19T08:12:16Z

pkg/base1/test-format.ts

@@ -1,5 +1,5 @@
 import cockpit from "cockpit";
-import QUnit from "qunit-tests";
+import QUnit, { f } from "qunit-tests";


What is this f? This aint no Python

jelly · 2024-04-19T08:13:50Z

pkg/lib/cockpit.js

+        // compat API: we used to accept 'factor' as a separate second arg
+        if (third_arg || (second_arg && !is_object(second_arg))) {
+            if (!deprecated_format_warned) {
+                console.warn(`cockpit.format_{bytes,bits}[_per_sec](..., ${second_arg}, ${third_arg}) is deprecated.`);


When do we remove it, should we include that? Like in 10 releases this API is gone?

I'm OK with next release already...

allisonkarlitskaya requested review from jelly and martinpitt April 18, 2024 10:48

allisonkarlitskaya mentioned this pull request Apr 18, 2024

test: port test-format to typescript #20322

Closed

github-advanced-security bot found potential problems Apr 18, 2024

View reviewed changes

pkg/lib/cockpit.d.ts Fixed Show fixed Hide fixed

allisonkarlitskaya force-pushed the format-ts branch from 9fd6155 to 72ff7c1 Compare April 18, 2024 10:57

allisonkarlitskaya force-pushed the format-ts branch from 72ff7c1 to b06cd03 Compare April 18, 2024 11:20

allisonkarlitskaya mentioned this pull request Apr 18, 2024

Rework display of current container memory usage cockpit-project/cockpit-podman#1673

Merged

1 task

martinpitt requested changes Apr 18, 2024

View reviewed changes

jelly reviewed Apr 18, 2024

View reviewed changes

allisonkarlitskaya force-pushed the format-ts branch from b06cd03 to 99dff27 Compare April 18, 2024 13:11

allisonkarlitskaya added 3 commits April 18, 2024 17:31

storaged: simplify utils.get_byte_units()

0e06f2c

Thanks to Martin for some ideas there.

allisonkarlitskaya added 3 commits April 18, 2024 18:27

storage: add a testcase for format_fsys_usage()

d254625

Let's make sure we don't regress this with changes to `cockpit.format_units()`.

test: port test-format to typescript

3ca2dcb

This was pretty much completely pain-free, with a minor exception: the test overrides the `cockpit.language` attribute, which we would otherwise probably declare as `const`.

allisonkarlitskaya force-pushed the format-ts branch from 99dff27 to 3ca2dcb Compare April 18, 2024 16:28

martinpitt approved these changes Apr 19, 2024

View reviewed changes

martinpitt requested a review from jelly April 19, 2024 05:44

jelly reviewed Apr 19, 2024

View reviewed changes

allisonkarlitskaya merged commit 850feb2 into cockpit-project:main Apr 19, 2024
80 checks passed

allisonkarlitskaya deleted the format-ts branch April 19, 2024 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up and type number formatting APIs #20334

Clean up and type number formatting APIs #20334

allisonkarlitskaya commented Apr 18, 2024

allisonkarlitskaya commented Apr 18, 2024

martinpitt left a comment

martinpitt Apr 18, 2024

allisonkarlitskaya Apr 18, 2024

jelly Apr 18, 2024

allisonkarlitskaya Apr 18, 2024

martinpitt Apr 18, 2024

martinpitt commented Apr 18, 2024

allisonkarlitskaya commented Apr 18, 2024 •

edited

Loading

martinpitt left a comment

martinpitt Apr 19, 2024

allisonkarlitskaya Apr 19, 2024

martinpitt Apr 19, 2024

jelly Apr 19, 2024

jelly Apr 19, 2024

allisonkarlitskaya Apr 19, 2024

Clean up and type number formatting APIs #20334

Clean up and type number formatting APIs #20334

Conversation

allisonkarlitskaya commented Apr 18, 2024

allisonkarlitskaya commented Apr 18, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Apr 18, 2024

allisonkarlitskaya commented Apr 18, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Apr 18, 2024 •

edited

Loading