[PATCH v4 1/3] Add support for structured output formatters.
Tomi Ollila
tomi.ollila at iki.fi
Thu Jul 12 05:05:14 PDT 2012
On Thu, Jul 12 2012, Mark Walters <markwalters1009 at gmail.com> wrote:
> On Thu, 12 Jul 2012, craven at gmx.net wrote:
>>> what is the advantage of having this as one function rather than end_map
>>> and end_list? Indeed, my choice (but I think most other people would
>>> disagree) would be to have both functions but still keep state as this
>>> currently does and then throw an error if the code closes the wrong
>>> thing.
>>
>> There's probably no advantage, one way or the other is fine, I'd say.
>> I've thought about introducing checks into the formatter functions, to
>> raise errors for improper closing, map_key not inside a map and things
>> like that, I just wasn't sure that would be acceptable.
>
> I will leave others to comment.
I like the current implementation -- record what has been opened and
have a common closing function which knows what it is closing. Less
checking in the code (i.e. less possible branches).
This makes the use of the interface a bit less self-documenting as
'end' is used instead of list/hash end (defining macros for this
documentation purposes would be really dumb thing to do ;)
What needs to be checked is that software doesn't attempt to 'end'
too many contexts (i quess it's doing it already). Any other output
errors (like forgetting to 'end' some blocks should be taken care
by proper test cases).
>>> A second question: do you have an implementation in this style for
>>> s-expressions. I find it hard to tell whether the interface is right
>>> with just a single example. Even a completely hacky not ready for review
>>> example would be helpful.
>>
>> See the attached patch :)
>
> This looks great. I found it much easier to review by splitting
> sprinter.c into two files sprinter-json.c and sprinter-sexp.c and then
> running meld on them. The similarity is then very clear. It might be
> worth submitting them as two files, but I leave other people to comment.
I like that -- is there (currently) need for splinter.c for common code ?
(splinter.h is definitely needed).
> (Doing so made some of the difference between json and s-exp clear: like
> that keys in mapkeys are quoted in json but not in s-exp)
>
> It could be that some of the code could be merged, but I am not sure
> that there is much advantage. I would imagine that these two sprinter.c
> files would basically never change so there is not much risk of them
> diverging.
I was thinking the same when looking splinter code yesterday -- how to
have even more common code for json&sexp. Maybe there could be something
to do just for these 2 purposes but It requires more effort and might
add more complexity for humans to perceive. ATM I'd go with this interface
and see later if anyone wants to do/experiment more -- as you said the
risk of diverging is minimal -- and in case there are separate source
files for json & sexp diffing those will be easy.
> I wonder if it would be worth using aggregate_t for both rather than
> using the closing symbol for this purpose in the json output.
>
> In any case this patch answers my query: the new structure does
> generalise very easily to s-expressions!
>
> Best wishes
>
> Mark
Tomi
>
>
>
>>
>> Thanks for the suggestions!
>>
>> Peter
>> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001
>> From: <craven at gmx.net>
>> Date: Thu, 12 Jul 2012 10:17:05 +0200
>> Subject: [PATCH] Add an S-Expression output format.
>>
>> ---
>> notmuch-search.c | 7 ++-
>> sprinter.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> sprinter.h | 4 ++
>> 3 files changed, 180 insertions(+), 1 deletion(-)
>>
>> diff --git a/notmuch-search.c b/notmuch-search.c
>> index b853f5f..f8bea9b 100644
>> --- a/notmuch-search.c
>> +++ b/notmuch-search.c
>> @@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,
>>
>> if (format != sprinter_text) {
>> format->begin_list (format);
>> + format->frame (format);
>> }
>>
>> for (i = 0;
>> @@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>> int exclude = EXCLUDE_TRUE;
>> unsigned int i;
>>
>> - enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
>> + enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }
>> format_sel = NOTMUCH_FORMAT_TEXT;
>>
>> notmuch_opt_desc_t options[] = {
>> @@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>> { NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
>> (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
>> { "text", NOTMUCH_FORMAT_TEXT },
>> + { "sexp", NOTMUCH_FORMAT_SEXP },
>> { 0, 0 } } },
>> { NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
>> (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
>> @@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>> case NOTMUCH_FORMAT_JSON:
>> format = sprinter_json_new (ctx, stdout);
>> break;
>> + case NOTMUCH_FORMAT_SEXP:
>> + format = sprinter_sexp_new (ctx, stdout);
>> + break;
>> }
>>
>> config = notmuch_config_open (ctx, NULL, NULL);
>> diff --git a/sprinter.c b/sprinter.c
>> index 649f79a..fce0f9b 100644
>> --- a/sprinter.c
>> +++ b/sprinter.c
>> @@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)
>> res->stream = stream;
>> return &res->vtable;
>> }
>> +
>> +/*
>> + * Every below here is the implementation of the SEXP printer.
>> + */
>> +
>> +typedef enum { MAP, LIST } aggregate_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;
>> +};
>> +
>> +struct sexp_state
>> +{
>> + struct sexp_state *parent;
>> + /* True if nothing has been printed in this aggregate yet.
>> + * Suppresses the comma before a value. */
>> + notmuch_bool_t first;
>> + /* The character that closes the current aggregate. */
>> + aggregate_t type;
>> +};
>> +
>> +static struct sprinter_sexp *
>> +sexp_begin_value(struct sprinter *sp)
>> +{
>> + struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> + if (spsx->state) {
>> + if (!spsx->state->first)
>> + fputc (' ', spsx->stream);
>> + else
>> + spsx->state->first = false;
>> + }
>> + return spsx;
>> +}
>> +
>> +static void
>> +sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)
>> +{
>> + struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> + struct sexp_state *state = talloc (spsx, struct sexp_state);
>> +
>> + fputc ('(', spsx->stream);
>> + state->parent = spsx->state;
>> + state->first = true;
>> + spsx->state = state;
>> + state->type = type;
>> +}
>> +
>> +static void
>> +sexp_begin_map(struct sprinter *sp)
>> +{
>> + sexp_begin_aggregate (sp, MAP);
>> +}
>> +
>> +static void
>> +sexp_begin_list(struct sprinter *sp)
>> +{
>> + sexp_begin_aggregate (sp, LIST);
>> +}
>> +
>> +static void
>> +sexp_end(struct sprinter *sp)
>> +{
>> + struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> + struct sexp_state *state = spsx->state;
>> +
>> + fputc (')', spsx->stream);
>> + spsx->state = state->parent;
>> + talloc_free (state);
>> + if(spsx->state == NULL)
>> + fputc ('\n', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_string(struct sprinter *sp, const char *val)
>> +{
>> + static const char * const escapes[] = {
>> + ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
>> + ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t"
>> + };
>> + struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> + fputc ('"', spsx->stream);
>> + for (; *val; ++val) {
>> + unsigned char ch = *val;
>> + if (ch < ARRAY_SIZE(escapes) && escapes[ch])
>> + fputs (escapes[ch], spsx->stream);
>> + else if (ch >= 32)
>> + fputc (ch, spsx->stream);
>> + else
>> + fprintf (spsx->stream, "\\u%04x", ch);
>> + }
>> + fputc ('"', spsx->stream);
>> + if (spsx->state != NULL && spsx->state->type == MAP)
>> + fputc (')', spsx->stream);
>> + spsx->state->first = false;
>> +}
>> +
>> +static void
>> +sexp_integer(struct sprinter *sp, int val)
>> +{
>> + struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> + fprintf (spsx->stream, "%d", val);
>> + if (spsx->state != NULL && spsx->state->type == MAP)
>> + fputc (')', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_boolean(struct sprinter *sp, notmuch_bool_t val)
>> +{
>> + struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> + fputs (val ? "#t" : "#f", spsx->stream);
>> + if (spsx->state != NULL && spsx->state->type == MAP)
>> + fputc (')', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_null(struct sprinter *sp)
>> +{
>> + struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> + fputs ("'()", spsx->stream);
>> + spsx->state->first = false;
>> +}
>> +
>> +static void
>> +sexp_map_key(struct sprinter *sp, const char *key)
>> +{
>> + struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> + fputc ('(', spsx->stream);
>> + fputs (key, spsx->stream);
>> + fputs (" . ", spsx->stream);
>> + spsx->state->first = true;
>> +}
>> +
>> +static void
>> +sexp_frame(struct sprinter *sp)
>> +{
>> + struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> + fputc ('\n', spsx->stream);
>> +}
>> +
>> +struct sprinter *
>> +sprinter_sexp_new(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,
>> + .string = sexp_string,
>> + .integer = sexp_integer,
>> + .boolean = sexp_boolean,
>> + .null = sexp_null,
>> + .map_key = sexp_map_key,
>> + .frame = sexp_frame,
>> + }
>> + };
>> + struct sprinter_sexp *res;
>> +
>> + res = talloc (ctx, struct sprinter_sexp);
>> + if (!res)
>> + return NULL;
>> +
>> + *res = template;
>> + res->stream = stream;
>> + return &res->vtable;
>> +}
>> diff --git a/sprinter.h b/sprinter.h
>> index 1dad9a0..a89eaa5 100644
>> --- a/sprinter.h
>> +++ b/sprinter.h
>> @@ -40,6 +40,10 @@ typedef struct sprinter
>> struct sprinter *
>> sprinter_json_new(const void *ctx, FILE *stream);
>>
>> +/* Create a new structure printer that emits S-Expressions */
>> +struct sprinter *
>> +sprinter_sexp_new(const void *ctx, FILE *stream);
>> +
>> /* A dummy structure printer that signifies that standard text output is
>> * to be used instead of any structured format.
>> */
>> --
>> 1.7.11.1
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list