On 12/21/21 14:47, Richard W.M. Jones wrote:
> On Mon, Dec 20, 2021 at 02:56:40PM +0100, Laszlo Ersek wrote:
>> + if (need_actual_sizes) then
>
> You don't need (and shouldn't use) the parentheses here.
Ah, you are totally right -- "muscle memory" from C. I've been getting
better at not using parens with "if"s in OCaml, but I still fail to
catch myself sometimes. Funnily enough, in this single patch, I wrote
*both* "if need_actual_sizes then" *and* "if (need_actual_sizes) then".
>
>> + (* Ovirt-engine considers the "ovf:actual_size" attribute
>> mandatory.
>> + * If we don't know the actual size, we must create the attribute
>> + * with empty contents.
>> + *)
>> + List.push_back attrs
>> + ("ovf:actual_size",
>> + match actual_size with
>> + | None -> ""
>> + | Some actual_size -> Int64.to_string (bytes_to_gb
>> actual_size)
>
> FYI I was going to suggest (untested!):
>
> actual_size |>
> Option.map (bytes_to_gb |> Int64.to_string) |>
> Option.default ""
Interesting, thanks for teaching me this!
... after consulting <https://ocaml.org/api/Option.html>, is the last
function "Option.value", rather than "Option.default"?
>
> but actually once I wrote it out like this it's pretty clunky and
> obscure. If the match was less complex it could make sense to
> consider this, but I don't think in this case.
>
> Rest of the patch is all good, thanks, so:
>
> Reviewed-by: Richard W.M. Jones <[email protected]>
I've removed the superfluous parens, and have re-run "make check".
$ git range-diff --color \
master \
restrict-actual-size-to-rhv-upload-rhbz-2034240-v1 \
apply
> 1: 9695e6f2394f ! 1: 07b12fe99fb9 output_rhv: restrict block status
> collection to the old RHV output
> @@ -23,6 +23,10 @@
> Suggested-by: Richard W.M. Jones <[email protected]>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240
> Signed-off-by: Laszlo Ersek <[email protected]>
> + Message-Id: <[email protected]>
> + Tested-by: Nir Soffer <[email protected]>
> + [[email protected]: drop parens around condition in "if"; thanks:
> Rich]
> + Reviewed-by: Richard W.M. Jones <[email protected]>
>
> diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli
> --- a/lib/create_ovf.mli
> @@ -97,7 +101,7 @@
> - | None -> ""
> - | Some actual_size -> Int64.to_string (bytes_to_gb
> actual_size)
> - );
> -+ if (need_actual_sizes) then
> ++ if need_actual_sizes then
> + (* Ovirt-engine considers the "ovf:actual_size" attribute
> mandatory.
> + * If we don't know the actual size, we must create the
> attribute
> + * with empty contents.
Commit 07b12fe99fb9.
Thanks!
Laszlo
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
