Pino Toscano
2014-Jan-23 09:18 UTC
Re: [Libguestfs] [PATCH] builder: read all the available notes from the index
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
Richard W.M. Jones
2014-Jan-23 12:01 UTC
Re: [Libguestfs] [PATCH] builder: read all the available notes from the index
On Thu, Jan 23, 2014 at 10:18:22AM +0100, Pino Toscano wrote:> 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.OK, got it. Can you still change the patch to avoid the _ match case. I think something like this should work: | { Index_parser.notes = ("", notes) :: _ } -> (* print notes *) | { Index_parser.notes = _ :: _ } | { Index_parser.notes = [] } -> (* no untranslated notes *) ACK with that change. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Pino Toscano
2014-Jan-23 14:39 UTC
Re: [Libguestfs] [PATCH] builder: read all the available notes from the index
On Thursday 23 January 2014 12:01:02 Richard W.M. Jones wrote:> Can you still change the patch to avoid the _ match > case. I think something like this should work: > | { Index_parser.notes = ("", notes) :: _ } -> (* print notes *) > | { Index_parser.notes = _ :: _ } > | { Index_parser.notes = [] } -> (* no untranslated notes *) > > ACK with that change.OK for me. Modified accordingly, and pushed. -- Pino Toscano
Possibly Parallel Threads
- Re: [PATCH] builder: read all the available notes from the index
- Re: [PATCH] builder: read all the available notes from the index
- [PATCH] builder: read all the available notes from the index
- [PATCH] builder: output translated notes
- [PATCH 06/10] builder: split Index_parser.index in an own module