Libguestfs - Jan 2014 - Re: [PATCH] builder: read all the available notes from the index

Switch the internal storage for the notes of each entry to a sorted list
with all the subkeys available (which should represent the translations
to various languages).
The current outputs are the same (i.e. still the untranslated notes), so
this is just internal refactoring/preparation.
---
 builder/builder.ml       |  4 ++--
 builder/index_parser.ml  | 19 +++++++++++++++----
 builder/index_parser.mli |  2 +-
 builder/list_entries.ml  | 10 +++++++---
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/builder/builder.ml b/builder/builder.ml
index bb0b108..19d1e42 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -200,9 +200,9 @@ let main ()    (match mode with
   | `Notes ->                           (* --notes *)
     (match entry with
-    | { Index_parser.notes = Some notes } ->
+    | { Index_parser.notes = ("", notes) :: _ } ->
       print_endline notes;
-    | { Index_parser.notes = None } ->
+    | { Index_parser.notes = _ } ->
       printf (f_"There are no notes for %s\n") arg
     );
     exit 0
diff --git a/builder/index_parser.ml b/builder/index_parser.ml
index da44b21..961b91b 100644
--- a/builder/index_parser.ml
+++ b/builder/index_parser.ml
@@ -35,7 +35,7 @@ and entry = {
   compressed_size : int64 option;
   expand : string option;
   lvexpand : string option;
-  notes : string option;
+  notes : (string * string) list;
   hidden : bool;
 
   sigchecker : Sigchecker.t;
@@ -92,8 +92,8 @@ let print_entry chan (name, { printable_name = printable_name;
   | Some lvexpand -> fp "lvexpand=%s\n" lvexpand
   );
   (match notes with
-  | None -> ()
-  | Some notes -> fp "notes=%s\n" notes
+  | ("", notes) :: _ -> fp "notes=%s\n" notes
+  | _ -> ()
   );
   if hidden then fp "hidden=true\n"
 
@@ -219,7 +219,18 @@ let get_index ~prog ~debug ~downloader ~sigchecker source  
let lvexpand              try Some (List.assoc ("lvexpand", None)
fields) with Not_found -> None in
           let notes -            try Some (List.assoc ("notes", None)
fields) with Not_found -> None in
+            let rec loop = function
+              | [] -> []
+              | (("notes", subkey), value) :: xs ->
+                let subkey = match subkey with
+                | None -> ""
+                | Some v -> v in
+                (subkey, value) :: loop xs
+              | _ :: xs -> loop xs in
+            List.sort (
+              fun (k1, _) (k2, _) ->
+                String.compare k1 k2
+            ) (loop fields) in
           let hidden              try bool_of_string (List.assoc
("hidden", None) fields)
             with
diff --git a/builder/index_parser.mli b/builder/index_parser.mli
index 54f1807..3c679b3 100644
--- a/builder/index_parser.mli
+++ b/builder/index_parser.mli
@@ -29,7 +29,7 @@ and entry = {
   compressed_size : int64 option;
   expand : string option;
   lvexpand : string option;
-  notes : string option;
+  notes : (string * string) list;
   hidden : bool;
 
   sigchecker : Sigchecker.t;
diff --git a/builder/list_entries.ml b/builder/list_entries.ml
index 7369e6c..742e43b 100644
--- a/builder/list_entries.ml
+++ b/builder/list_entries.ml
@@ -71,10 +71,10 @@ and list_entries_long ~sources index            printf
"%-24s %s\n" (s_"Download size:") (human_size size);
         );
         (match notes with
-        | None -> ()
-        | Some notes ->
+        | ("", notes) :: _ ->
           printf "\n";
           printf (f_"Notes:\n\n%s\n") notes
+        | _ -> ()
         );
         printf "\n"
       )
@@ -108,6 +108,10 @@ and list_entries_json ~sources index      | None -> ()
     | Some n ->
       printf "    \"%s\": \"%Ld\",\n" key n in
+  let print_notes = function
+    | ("", notes) :: _ ->
+      printf "    \"notes\": \"%s\",\n"
(json_string_escape notes)
+    | _ -> () in
 
   printf "{\n";
   printf "  \"version\": %d,\n" 1;
@@ -132,7 +136,7 @@ and list_entries_json ~sources index       
json_optional_printf_string "full-name" printable_name;
       printf "    \"size\": %Ld,\n" size;
       json_optional_printf_int64 "compressed-size" compressed_size;
-      json_optional_printf_string "notes" notes;
+      print_notes notes;
       printf "    \"hidden\": %s\n" (json_string_of_bool
hidden);
       printf "  }%s\n" (trailing_comma i (List.length index))
   ) index;
-- 
1.8.3.1

On Wed, Jan 22, 2014 at 11:22:47PM +0100, Pino Toscano
wrote:
> Switch the internal storage for the notes of each entry to a sorted list
> with all the subkeys available (which should represent the translations
> to various languages).
> The current outputs are the same (i.e. still the untranslated notes), so
> this is just internal refactoring/preparation.
> ---
>  builder/builder.ml       |  4 ++--
>  builder/index_parser.ml  | 19 +++++++++++++++----
>  builder/index_parser.mli |  2 +-
>  builder/list_entries.ml  | 10 +++++++---
>  4 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/builder/builder.ml b/builder/builder.ml
> index bb0b108..19d1e42 100644
> --- a/builder/builder.ml
> +++ b/builder/builder.ml
> @@ -200,9 +200,9 @@ let main () >    (match mode with
>    | `Notes ->                           (* --notes *)
>      (match entry with
> -    | { Index_parser.notes = Some notes } ->
> +    | { Index_parser.notes = ("", notes) :: _ } ->
>        print_endline notes;
> -    | { Index_parser.notes = None } ->
> +    | { Index_parser.notes = _ } ->
If you have to match on _, especially:
> --- a/builder/list_entries.ml
> +++ b/builder/list_entries.ml
> @@ -71,10 +71,10 @@ and list_entries_long ~sources index >           
printf "%-24s %s\n" (s_"Download size:") (human_size size);
>          );
>          (match notes with
> -        | None -> ()
> -        | Some notes ->
> +        | ("", notes) :: _ ->
>            printf "\n";
>            printf (f_"Notes:\n\n%s\n") notes
> +        | _ -> ()
then you've done something wrong.  The problem is that the _ stops the
compiler from finding missing cases in your code, since _ matches any
case.  It's a bit like why it's bad practice to catch any exception in
Java.

Since the list is going to contain something like one of these:

  [ "", "some notes" ]
  [ "en", "some notes"; "ru", "..." ]
  [ "ru", "..."; "", "..." ]
  []

the pattern
  | ("", notes) :: _ ->
is only going to match the first of those, not the second or third.

I think for this refactoring what you really want is:

  | [_, notes] :: _ -> (* print the notes *)
  | [] -> (* no notes *)

which will print the first set of notes that happen to be in the index
(in any language), which is fine for the current index that only has
one set of notes per template.

Later when you actually implement language support you're going to
have to use List.assoc, so:

  | notes when List.mem_assoc "en" notes ->
     printf "notes = %s" (List.assoc "en" notes)
  | [] ->
     printf "no notes"

(or something cleverer to deal with the current locale).

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/

On Wednesday 22 January 2014 22:51:02 Richard W.M. Jones
wrote:
> On Wed, Jan 22, 2014 at 11:22:47PM +0100, Pino Toscano wrote:
> > Switch the internal storage for the notes of each entry to a sorted
> > list with all the subkeys available (which should represent the
> > translations to various languages).
> > The current outputs are the same (i.e. still the untranslated
> > notes), so this is just internal refactoring/preparation.
> > ---
> > 
> >  builder/builder.ml       |  4 ++--
> >  builder/index_parser.ml  | 19 +++++++++++++++----
> >  builder/index_parser.mli |  2 +-
> >  builder/list_entries.ml  | 10 +++++++---
> >  4 files changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/builder/builder.ml b/builder/builder.ml
> > index bb0b108..19d1e42 100644
> > --- a/builder/builder.ml
> > +++ b/builder/builder.ml
> > @@ -200,9 +200,9 @@ let main () > > 
> >    (match mode with
> >    
> >    | `Notes ->                           (* --notes *)
> >    | 
> >      (match entry with
> > 
> > -    | { Index_parser.notes = Some notes } ->
> > +    | { Index_parser.notes = ("", notes) :: _ } ->
> > 
> >        print_endline notes;
> > 
> > -    | { Index_parser.notes = None } ->
> > +    | { Index_parser.notes = _ } ->
> 
> If you have to match on _, especially:
> > --- a/builder/list_entries.ml
> > +++ b/builder/list_entries.ml
> > @@ -71,10 +71,10 @@ and list_entries_long ~sources index > > 
> >            printf "%-24s %s\n" (s_"Download
size:") (human_size
> >            size);
> >          
> >          );
> >          (match notes with
> > 
> > -        | None -> ()
> > -        | Some notes ->
> > +        | ("", notes) :: _ ->
> > 
> >            printf "\n";
> >            printf (f_"Notes:\n\n%s\n") notes
> > 
> > +        | _ -> ()
> 
> then you've done something wrong.  The problem is that the _ stops the
> compiler from finding missing cases in your code, since _ matches any
> case.  It's a bit like why it's bad practice to catch any exception
> in Java.
Yes, I know that.
> Since the list is going to contain something like one of these:
> 
>   [ "", "some notes" ]
>   [ "en", "some notes"; "ru", "..."
]
>   [ "ru", "..."; "", "..." ]
>   []
> 
> the pattern
> 
>   | ("", notes) :: _ ->
> 
> is only going to match the first of those, not the second or third.
If you notice the notes reading, I'm sorting the list by
"language", so
the empty string (= no translation) should always be the first, if 
present. So your lists above would actually be:

  [ "", "some notes" ]
  [ "en", "some notes"; "ru", "..." ]
  [ "", "..."; "ru", "..." ]
  []

I did that to just refactor without actually changing the behaviour, 
that is show the untranslated notes, thus also printing nothing if there 
are notes[..]= entries but no notes= one.
Other than that, sorting the list will help later when printing all the 
translated notes to JSON.
> Later when you actually implement language support you're going to
> 
> have to use List.assoc, so:
>   | notes when List.mem_assoc "en" notes ->
> 
>      printf "notes = %s" (List.assoc "en" notes)
> 
>   | [] ->
> 
>      printf "no notes"
> 
> (or something cleverer to deal with the current locale).
Yes, that's my step following this one, i.e. print the right language 
for the current locale when the output is internationalised, and all the 
languages in the JSON output.

-- 
Pino Toscano