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: <874nk5ansj.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>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 01 Dec 2012 14:26:46 +0000
Message-ID: <87k3t1etll.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, 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.

Best wishes

Mark

---
 sprinter-sexp.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 sprinter.h      |    6 +++-
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/sprinter-sexp.c b/sprinter-sexp.c
index 8401c52..51bab68 100644
--- a/sprinter-sexp.c
+++ b/sprinter-sexp.c
@@ -1,11 +1,16 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <talloc.h>
 #include "sprinter.h"
 
+typedef enum {
+    SEXP_ALIST,
+    SEXP_PLIST
+} sexp_list_t;
+
 struct sprinter_sexp {
     struct sprinter vtable;
     FILE *stream;
     /* Top of the state stack, or NULL if the printer is not currently
      * inside any aggregate types. */
     struct sexp_state *state;
@@ -81,26 +86,39 @@ static void
 sexp_begin_list (struct sprinter *sp)
 {
     sexp_begin_aggregate (sp, '(', ')');
 }
 
 static void
-sexp_end (struct sprinter *sp)
+sexp_end (struct sprinter *sp, sexp_list_t list_type)
 {
     struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
     struct sexp_state *state = sps->state;
 
-    if (sps->state->in_map)
-        fputc (')', sps->stream);
+    if (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);
     if (sps->state == NULL)
         fputc ('\n', sps->stream);
 }
 
+static void
+sexp_end_alist (struct sprinter *sp)
+{
+    sexp_end(sp, SEXP_ALIST);
+}
+
+static void
+sexp_end_plist (struct sprinter *sp)
+{
+    sexp_end(sp, SEXP_PLIST);
+}
+
 /* This implementation supports embedded NULs as allowed by the JSON
  * specification and Unicode.  Support for *parsing* embedded NULs
  * varies, but is generally not a problem outside of C-based parsers
  * (Python's json module and Emacs' json.el take embedded NULs in
  * stride). */
 static void
@@ -177,24 +195,36 @@ sexp_null (struct sprinter *sp)
     struct sprinter_sexp *sps = sexp_begin_value (sp);
 
     fputs ("nil", sps->stream);
 }
 
 static void
-sexp_map_key (struct sprinter *sp, const char *key)
+sexp_map_key_alist (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);
     sexp_symbol (sp, key);
     fputc (' ', sps->stream);
 }
 
 static void
+sexp_map_key_plist (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);
+    sexp_symbol (sp, key);
+    fputc (' ', sps->stream);
+}
+
+static void
 sexp_set_prefix (unused (struct sprinter *sp), unused (const char *name))
 {
 }
 
 static void
 sexp_separator (struct sprinter *sp)
@@ -202,25 +232,56 @@ sexp_separator (struct sprinter *sp)
     struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;
 
     sps->insert_separator = TRUE;
 }
 
 struct sprinter *
-sprinter_sexp_create (const void *ctx, FILE *stream)
+sprinter_sexp_alist_create (const void *ctx, FILE *stream)
+{
+    static const struct sprinter_sexp template = {
+        .vtable = {
+            .begin_map = sexp_begin_map,
+            .begin_list = sexp_begin_list,
+            .end = sexp_end_alist,
+            .string = sexp_string,
+            .string_len = sexp_string_len,
+            .integer = sexp_integer,
+            .boolean = sexp_boolean,
+            .null = sexp_null,
+            .map_key = sexp_map_key_alist,
+            .separator = sexp_separator,
+            .set_prefix = sexp_set_prefix,
+            .is_text_printer = FALSE,
+        }
+    };
+    struct sprinter_sexp *res;
+
+    res = talloc (ctx, struct sprinter_sexp);
+    if (! res)
+        return NULL;
+
+    *res = template;
+    res->stream = stream;
+    return &res->vtable;
+}
+
+
+struct sprinter *
+sprinter_sexp_plist_create (const void *ctx, FILE *stream)
 {
     static const struct sprinter_sexp template = {
         .vtable = {
             .begin_map = sexp_begin_map,
             .begin_list = sexp_begin_list,
-            .end = sexp_end,
+            .end = sexp_end_plist,
             .string = sexp_string,
             .string_len = sexp_string_len,
             .integer = sexp_integer,
             .boolean = sexp_boolean,
             .null = sexp_null,
-            .map_key = sexp_map_key,
+            .map_key = sexp_map_key_plist,
             .separator = sexp_separator,
             .set_prefix = sexp_set_prefix,
             .is_text_printer = FALSE,
         }
     };
     struct sprinter_sexp *res;
diff --git a/sprinter.h b/sprinter.h
index 59776a9..9a496cd 100644
--- a/sprinter.h
+++ b/sprinter.h
@@ -69,9 +69,13 @@ sprinter_text_create (const void *ctx, FILE *stream);
 /* Create a new structure printer that emits JSON. */
 struct sprinter *
 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