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