[Libguestfs] [PATCH] builder: output translated notes

[Richard W.M. Jones]

On Thu, Jan 30, 2014 at 02:44:12PM +0100, Pino Toscano wrote:
> +  let l = ref [] in

'xs' is a bit more natural for lists of things than 'l'.

> +  if Str.string_match regex loc 0 then (
> +    let match_or_empty n =
> +      try Str.matched_group n loc with
> +      | Not_found -> "" in

My preference is to put 'in' on a separate line if you're defining a
function (as here), and 'in' at the end of the line if you're defining
a value.

So I would have written this:

  let match_or_empty n =
    try Str.matched_group n loc with Not_found -> ""
   in

> +        let notes = List.fold_left (
> +          fun acc lang ->
> +            match List.filter (
> +              fun (langkey, _) ->
> +                match langkey with
> +                | "C" -> lang = ""
> +                | langkey -> langkey = lang
> +            ) notes with
> +            | (_, noteskey) :: _ -> noteskey :: acc
> +            | [] -> acc
> +        ) [] langs in

I've noticed you like to put huge expressions between match .. with!

Let bindings can be nested arbitrarily, and are scoped to just the
current level, so this might be easier to read:

  fun acc lang ->
    let notes = List.filter (
      function
      | ("C", _) -> lang = ""
      | (langkey, _) -> lang = langkey
    ) notes in
    match notes with
    ...etc...

Rest seems fine so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/