4:19 a.m.
On 01/06/22 16:02, Richard W.M. Jones wrote:
On Thu, Jan 06, 2022 at 03:09:08PM +0100, Laszlo Ersek wrote:
> For debugging purposes, we'll want to print the list of devices returned
> by the previously introduced "osinfo_os#get_devices" method.
>
> Format the device list as a nice table.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1942325
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
>
> Notes:
> Log examples will be shown later in this series, in the Notes section(s)
> of other patch(es).
>
> convert/libosinfo_utils.mli | 3 ++
> convert/libosinfo_utils.ml | 35 ++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/convert/libosinfo_utils.mli b/convert/libosinfo_utils.mli
> index b3714d224ae4..5a703334d9f0 100644
> --- a/convert/libosinfo_utils.mli
> +++ b/convert/libosinfo_utils.mli
> @@ -27,3 +27,6 @@ val get_os_by_short_id : string -> Libosinfo.osinfo_os
>
> val string_of_osinfo_device_driver : Libosinfo.osinfo_device_driver -> string
> (** Convert a [osinfo_device_driver] to a printable string for debugging. *)
> +
> +val string_of_osinfo_device_list : Libosinfo.osinfo_device list -> string
> +(** Convert an [osinfo_device] list to a printable string for debugging. *)
> diff --git a/convert/libosinfo_utils.ml b/convert/libosinfo_utils.ml
> index 1fc138cce321..d5eb082b458c 100644
> --- a/convert/libosinfo_utils.ml
> +++ b/convert/libosinfo_utils.ml
> @@ -42,3 +42,38 @@ let string_of_osinfo_device_driver { Libosinfo.architecture;
location;
> (if signed then "signed" else "unsigned")
> priority
> (String.concat " " files)
> +
> +let string_of_osinfo_device_list dev_list =
> +
> + (* Turn the fields of an "osinfo_device" record into a list. *)
> + let listify { Libosinfo.id; vendor; vendor_id; product; product_id; name;
> + class_; bus_type; subsystem } =
> + [ id; vendor; vendor_id; product; product_id; name;
> + class_; bus_type; subsystem ]
> +
> + (* Given a list of strings, and a list of previously known maximum widths,
> + * "increase" each width, if necessary, to the length of the
corresponding
> + * string.
> + *)
> + and grow_widths = List.map2 (fun s -> max (String.length s))
> + in
> +
> + (* Compute the maximum width for each field in "dev_list". *)
> + let max_widths =
> + List.fold_right grow_widths (List.map listify dev_list)
> + [ 0; 0; 0; 0; 0; 0; 0; 0; 0 ]
> +
> + (* Given a list of strings and a list of field widths, format "string1 |
> + * string2 | ... | stringN" such that each field is right-padded to the
> + * corresponding width.
> + *)
> + and columnate strings widths =
> + String.concat " | " (List.map2 (Printf.sprintf "%-*s")
widths strings)
> + in
> +
> + (* Format "dev_list" as a table by (a) printing one
"osinfo_device" record
> + * per line, and (b) right-padding each field of each "osinfo_device"
record
> + * to the maximum width of that field.
> + *)
> + String.concat "\n"
> + (List.map (fun dev -> columnate (listify dev) max_widths) dev_list)
Seems ... complicated, and I guess something that if we think we'll
need to format a lot of tables we should add a generic mechanism to
Std_utils.
It certainly occurred to me that we'd probably move this later to a more
central utils module.
I would have done this using printf a fixed maximum width like
this code:
https://gitlab.com/nbdkit/nbdkit/-/blob/d7c69529c453adb1b5b651803881dac2e...
I vaguely remember reviewing a patch from you that also used fixed
maximum widths... I think it was about printing nbd block statuses or
some such in a table format?... and I remember not really liking the
fixed widths. Fixed widths almost always do the wrong thing:
- they either waste space, or
- fail to tabulate correctly.
Case in point, before I set out writing this function, I ran:
osinfo-query device
just to get a taste of what I might expect.
If you run that command now, you'll see that the command attempts to
tabulate its output -- and that it fails at it *miserably*. The output
is utter garbage, so much so that I couldn't read it, before I did:
osinfo-query device | column -t -s \|
which helped at least a little.
So that's why I feel quite strongly about this -- and the resultant log
snippets (which you can see in the Notes section of the last patch in
the series) confirmed that determining the minimum sufficient field
widths was a good idea.
But it's encapsulated into a function and only used for debugging so
we could revise it later if we wanted, so sure.
Thanks! This is actually the most functional-style part of the series,
and I was super excited to write it. This is not the initial version: I
remembered that you emphasized partial application in OCaml, and I kept
trimming it (using partial application and existent List module APIs)
until I could take nothing more away.
Thanks!
Laszlo