From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, Peter Feigl <craven@gmx.net>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] Adding an S-expression structured output printer.
In-Reply-To: <87k3t1etll.fsf@qmul.ac.uk>
References: <1354264143-30173-1-git-send-email-craven@gmx.net>
	<871ufa9jpm.fsf@qmul.ac.uk> <m2txs6vwdy.fsf@guru.guru-group.fi>
	<87d2yu559y.fsf@qmul.ac.uk> <m2fw3pq4sf.fsf@guru.guru-group.fi>
	<874nk5ansj.fsf@qmul.ac.uk> <87k3t1etll.fsf@qmul.ac.uk>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 01 Dec 2012 15:46:32 +0000
Message-ID: <87vccl3hd3.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sat, 01 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Sat, 01 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>>> On Sat, Dec 01 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>>>
>>>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>>>>> On Sat, Dec 01 2012, Mark Walters wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Overall I like the series: I think I agree with all of Jani's
>>>>>> comments. 
>>>>>>
>>>>>> My one extra comment is that I think we should decide on whether we also
>>>>>> want a sexp plist version. I think we might want one for the emacs
>>>>>> front-end as that currently uses plists for everything.
>>>>>>
>>>>>> If we do we might want to change the names a little, both for functions
>>>>>> and options (eg sexp_a and sexp_p or something). Probably a lot of
>>>>>> sprinter-sexp would be common to both versions.
>>>>>
>>>>> This is an important question that needs to be addressed fast: options
>>>>> are:
>>>>>
>>>>> 1) have options to spit both alist & plist formats
>>>>> 2) when converting emacs to use s-expressions, convert it to use alists
>>>>> 3) start using plists instead of alists in Peter's android client
>>>>
>>>> Ok I have looked at this and the changes needed to output plist (or
>>>> both) are pretty small: the only functions from sprinter-sexp.c that
>>>> need to be changed are sexp_end and sexp_map_key. The total diff from
>>>> alist to plist is about 10 lines. I have a version which allows both
>>>> (the same sprinter file creates both possibilities) and have hooked it
>>>> into emacs/notmuch-show.el and it all seems to work. 
>>>>
>>>> (Search is more difficult as that uses the async parser; indeed even for
>>>> show I used sexp-at-point as suggested by Tomi which seems rather
>>>> underdocumented but does seem to work)
>>>>
>>>> Given the ease with which we can allow both I think that would be my
>>>> preference: the biggest problem is that slightly more cluttered option
>>>> list (i.e., we have to allow both --format=sexpa and --format=sexpp or
>>>> similar).
>>>>
>>>> (I can post the patch doing the above but almost all of it is modifying the
>>>> commands to choose alist or plist rather than modifying the
>>>> sprinter-sexp itself)
>>>
>>> As the diff is so small I agree that supporting 2 formats is good
>>> option.
>>>
>>> In case this is done I suggest that we proceed the following way:
>>>
>>> 1) Agree how we call these formats (sexpa & sexpp or something else)
>>> 2) Peter does his updates, including to call the format as will be decided
>>> 3) Mark posts his patches after Peter's work is pushed
>>
>> I think this seems a good approach. I should note that while the diff
>> between alist and plist is only about 10 lines I do need to duplicate
>> some of the surrounding code to allow both: its not huge but definitely
>> more than 10 lines. 
>>
>> I will clean it up a little and then post (as a diff on top of Peter's
>> patches) just so people can judge if it looks basically acceptable.
>
> Here is my current diff: note this is just the sprinter diff: it does
> not contain the plumbing into the cli functions. The largest part is the
> duplicated create function: I couldn't persuade the compiler to let me
> unify this (as the static const thing needs to be constant)
>
> The patch is -U6 as this makes it easier to see what is going on.

At Tomi's suggestion (on irc) here is a much nicer version of the change
needed for alist/plist.

Best wishes

Mark

---
 sprinter-sexp.c |   41 ++++++++++++++++++++++++++++++++++-------
 sprinter.h      |    6 +++++-
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/sprinter-sexp.c b/sprinter-sexp.c
index 8401c52..90ec9a0 100644
--- a/sprinter-sexp.c
+++ b/sprinter-sexp.c
@@ -3,6 +3,11 @@
 #include <talloc.h>
 #include "sprinter.h"
 
+typedef enum {
+    SEXP_ALIST,
+    SEXP_PLIST
+} sexp_list_t;
+
 struct sprinter_sexp {
     struct sprinter vtable;
     FILE *stream;
@@ -10,6 +15,8 @@ struct sprinter_sexp {
      * inside any aggregate types. */
     struct sexp_state *state;
 
+    /* A flag to say if the sprinter print alists or plists. */
+    sexp_list_t list_type;
     /* A flag to signify that a separator should be inserted in the
      * output as soon as possible.
      */
@@ -89,8 +96,9 @@ sexp_end (struct sprinter *sp)
     struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
     struct sexp_state *state = sps->state;
 
-    if (sps->state->in_map)
-        fputc (')', sps->stream);
+    if (sps->list_type == SEXP_ALIST)
+	if (sps->state->in_map)
+	    fputc (' ', sps->stream);
     fputc (sps->state->close, sps->stream);
     sps->state = state->parent;
     talloc_free (state);
@@ -184,9 +192,15 @@ sexp_map_key (struct sprinter *sp, const char *key)
 {
     struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
 
-    if( sps->state->in_map && ! sps->state->first)
-        fputs (") ", sps->stream);
-    fputc ('(', sps->stream);
+    if (sps->list_type == SEXP_ALIST) {
+	if( sps->state->in_map && ! sps->state->first)
+	    fputs (") ", sps->stream);
+	fputc ('(', sps->stream);
+    } else {
+	if( sps->state->in_map && ! sps->state->first)
+	    fputs (" ", sps->stream);
+	fputc (':', sps->stream);
+    }
     sexp_symbol (sp, key);
     fputc (' ', sps->stream);
 }
@@ -204,8 +218,8 @@ sexp_separator (struct sprinter *sp)
     sps->insert_separator = TRUE;
 }
 
-struct sprinter *
-sprinter_sexp_create (const void *ctx, FILE *stream)
+static struct sprinter *
+sprinter_sexp_create (const void *ctx, FILE *stream, sexp_list_t list_type)
 {
     static const struct sprinter_sexp template = {
         .vtable = {
@@ -231,5 +245,18 @@ sprinter_sexp_create (const void *ctx, FILE *stream)
 
     *res = template;
     res->stream = stream;
+    res->list_type = list_type;
     return &res->vtable;
 }
+
+struct sprinter *
+sprinter_sexp_alist_create (const void *ctx, FILE *stream)
+{
+    return sprinter_sexp_create (ctx, stream, SEXP_ALIST);
+}
+
+struct sprinter *
+sprinter_sexp_plist_create (const void *ctx, FILE *stream)
+{
+    return sprinter_sexp_create (ctx, stream, SEXP_PLIST);
+}
diff --git a/sprinter.h b/sprinter.h
index 59776a9..9a496cd 100644
--- a/sprinter.h
+++ b/sprinter.h
@@ -72,6 +72,10 @@ sprinter_json_create (const void *ctx, FILE *stream);
 
 /* Create a new structure printer that emits S-Expressions. */
 struct sprinter *
-sprinter_sexp_create (const void *ctx, FILE *stream);
+sprinter_sexp_alist_create (const void *ctx, FILE *stream);
+
+/* Create a new structure printer that emits S-Expressions. */
+struct sprinter *
+sprinter_sexp_plist_create (const void *ctx, FILE *stream);
 
 #endif // NOTMUCH_SPRINTER_H
-- 
1.7.9.1