[PATCH 1/3] Adding an S-expression structured output printer.
Mark Walters
markwalters1009 at gmail.com
Sat Dec 1 07:46:32 PST 2012
On Sat, 01 Dec 2012, Mark Walters <markwalters1009 at gmail.com> wrote:
> On Sat, 01 Dec 2012, Mark Walters <markwalters1009 at gmail.com> wrote:
>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila at iki.fi> wrote:
>>> On Sat, Dec 01 2012, Mark Walters <markwalters1009 at gmail.com> wrote:
>>>
>>>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila at 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
More information about the notmuch
mailing list