* Proposed structure formatter API @ 2012-06-30 2:59 Austin Clements 2012-07-12 7:43 ` Structured Formatters for JSON Output craven 0 siblings, 1 reply; 18+ messages in thread From: Austin Clements @ 2012-06-30 2:59 UTC (permalink / raw) To: Peter Feigl, Jameson Graef Rollins; +Cc: notmuch [-- Attachment #1: Type: text/plain, Size: 2551 bytes --] On IRC today, Peter renewed discussions of supporting an S-expression format, which I think would be a fantastic thing to have. A while back [1] I tried to explain an API I thought would form a good foundation for this by abstracting away the differences between JSON and S-expression output so both structured formats can use the same code, but I didn't express the idea very clearly. I built essentially this API for another project recently and it worked well (and I learned a few things), so I figured I would give it another shot. It's simple enough that I tossed together an example implementation, which is attached. The idea is similar to the existing format structures, but instead of dealing with complex, high-level concepts like threads or messages, this API deals with syntax-level concepts like lists and strings. All of the structured output formats can then be supported by a single high-level formatter backed by different syntax formatters. There are a few ways something like this could be integrated with the high-level formatters. For show, the least invasive thing would be to have two notmuch_show_format structures with different part functions. These part functions would be thin wrappers that simply create the right structure printer and then pass it to a common part function. It would be even nicer if we could get rid of the message_set_{start,sep,end} fields, since those duplicate functionality from the structure printer and their use is haphazard and overly complicated. We could do this by changing notmuch_show_format to something like typedef struct notmuch_show_format { struct sprinter *(*new_sprinter) (const void *ctx); notmuch_status_t (*part) (const void *ctx, struct sprinter *sp, struct mime_node *node, const struct notmuch_show_params *params); } notmuch_show_format_t; For the JSON and S-expression show formats, new_sprinter would be sprinter_json_new and sprinter_sexp_new, respectively, and they could share a part function. For the text, mbox, and raw formatters, new_sprinter could simply be NULL, or we could provide a "NULL structure printer" implementation that does nothing. We could do something similar for reply. search_format is more complicated, but might also benefit more. Most of those fields have to do with how to print lists and objects and could be removed if the format simply provided a new_sprinter method like the notmuch_show_format I suggested above. [1] id:"20120121220407.GK16740@mit.edu" [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: sprinter.c --] [-- Type: text/x-csrc, Size: 4987 bytes --] #include <stdbool.h> #include <stdio.h> #include <talloc.h> #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0])) /* Structure printer interface */ struct sprinter { void (*begin_map) (struct sprinter *); void (*begin_list) (struct sprinter *); void (*end) (struct sprinter *); void (*string) (struct sprinter *, const char *); void (*integer) (struct sprinter *, int); void (*boolean) (struct sprinter *, bool); void (*null) (struct sprinter *); void (*map_key) (struct sprinter *, const char *); void (*frame) (struct sprinter *); }; /* Create a new structure printer that emits JSON */ struct sprinter * sprinter_json_new(const void *ctx, FILE *stream); int main(int argc, char **argv) { struct sprinter *test = sprinter_json_new (NULL, stdout); test->begin_list (test); test->string (test, "test"); test->integer (test, 42); test->boolean (test, true); test->null (test); test->frame (test); test->begin_map (test); test->map_key (test, "Hello"); test->string (test, "world\n"); test->end (test); test->frame (test); test->end (test); talloc_free (test); } /* * Every below here is private implementation. */ struct sprinter_json { struct sprinter vtable; FILE *stream; /* Top of the state stack, or NULL if the printer is not currently * inside any aggregate types. */ struct json_state *state; }; struct json_state { struct json_state *parent; /* True if nothing has been printed in this aggregate yet. * Suppresses the comma before a value. */ bool first; /* The character that closes the current aggregate. */ char close; }; /* Helper function to set up the stream to print a value. If this * value follows another value, prints a comma. */ static struct sprinter_json * json_begin_value(struct sprinter *sp) { struct sprinter_json *spj = (struct sprinter_json*)sp; if (spj->state) { if (!spj->state->first) fputs (", ", spj->stream); else spj->state->first = false; } return spj; } /* Helper function to begin an aggregate type. Prints the open * character and pushes a new state frame. */ static void json_begin_aggregate(struct sprinter *sp, char open, char close) { struct sprinter_json *spj = json_begin_value (sp); struct json_state *state = talloc (spj, struct json_state); fputc (open, spj->stream); state->parent = spj->state; state->first = true; state->close = close; spj->state = state; } static void json_begin_map(struct sprinter *sp) { json_begin_aggregate (sp, '{', '}'); } static void json_begin_list(struct sprinter *sp) { json_begin_aggregate (sp, '[', ']'); } static void json_end(struct sprinter *sp) { struct sprinter_json *spj = (struct sprinter_json*)sp; struct json_state *state = spj->state; fputc (spj->state->close, spj->stream); spj->state = state->parent; talloc_free (state); if(spj->state == NULL) fputc ('\n', spj->stream); } static void json_string(struct sprinter *sp, const char *val) { const static char * const escapes[] = { ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b", ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t" }; struct sprinter_json *spj = json_begin_value (sp); fputc ('"', spj->stream); for (; *val; ++val) { unsigned char ch = *val; if (ch < ARRAY_SIZE(escapes) && escapes[ch]) fputs (escapes[ch], spj->stream); else if (ch >= 32) fputc (ch, spj->stream); else fprintf (spj->stream, "\\u%04x", ch); } fputc ('"', spj->stream); } static void json_integer(struct sprinter *sp, int val) { struct sprinter_json *spj = json_begin_value (sp); fprintf (spj->stream, "%d", val); } static void json_boolean(struct sprinter *sp, bool val) { struct sprinter_json *spj = json_begin_value (sp); fputs (val ? "true" : "false", spj->stream); } static void json_null(struct sprinter *sp) { struct sprinter_json *spj = json_begin_value (sp); fputs ("null", spj->stream); } static void json_map_key(struct sprinter *sp, const char *key) { struct sprinter_json *spj = (struct sprinter_json*)sp; json_string (sp, key); fputs (": ", spj->stream); spj->state->first = true; } static void json_frame(struct sprinter *sp) { struct sprinter_json *spj = (struct sprinter_json*)sp; fputc ('\n', spj->stream); } struct sprinter * sprinter_json_new(const void *ctx, FILE *stream) { const static struct sprinter_json template = { .vtable = { .begin_map = json_begin_map, .begin_list = json_begin_list, .end = json_end, .string = json_string, .integer = json_integer, .boolean = json_boolean, .null = json_null, .map_key = json_map_key, .frame = json_frame, } }; struct sprinter_json *res; res = talloc (ctx, struct sprinter_json); if (!res) return NULL; *res = template; res->stream = stream; return &res->vtable; } ^ permalink raw reply [flat|nested] 18+ messages in thread
* Structured Formatters for JSON Output 2012-06-30 2:59 Proposed structure formatter API Austin Clements @ 2012-07-12 7:43 ` craven 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: craven @ 2012-07-12 7:43 UTC (permalink / raw) To: notmuch Currently there is no easy way to add support for different structured formatters (like JSON). For example, adding support for S-Expressions would result in code duplication. This patch series amends the situation by introducing structured formatters, which allow different implementations of structured output for elements like lists, maps, strings and numbers. The new code in sprinter.h and sprinter.c can be used instead of the current ad-hoc output in all parts of notmuch, a patch for notmuch-search.c is included. In a later patch, all other parts of notmuch should be adapted to the structured formatters, and the creation of formatters should be centralised (to make adding new formatters easier). ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` Structured Formatters for JSON Output craven @ 2012-07-12 7:43 ` craven 2012-07-12 9:14 ` Mark Walters ` (4 more replies) 2012-07-12 7:43 ` [PATCH v4 2/3] Add structured output formatter for JSON craven 2012-07-12 7:43 ` [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search craven 2 siblings, 5 replies; 18+ messages in thread From: craven @ 2012-07-12 7:43 UTC (permalink / raw) To: notmuch This patch adds a new type sprinter_t, which is used for structured formatting, e.g. JSON or S-Expressions. The structure printer is the code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). The structure printer contains the following function pointers: /* start a new map/dictionary structure. */ void (*begin_map) (struct sprinter *); /* start a new list/array structure */ void (*begin_list) (struct sprinter *); /* end the last opened list or map structure */ void (*end) (struct sprinter *); /* print one string/integer/boolean/null element (possibly inside a * list or map */ void (*string) (struct sprinter *, const char *); void (*integer) (struct sprinter *, int); void (*boolean) (struct sprinter *, notmuch_bool_t); void (*null) (struct sprinter *); /* print the key of a map's key/value pair. */ void (*map_key) (struct sprinter *, const char *); /* print a frame delimiter (such as a newline or extra whitespace) */ void (*frame) (struct sprinter *); The printer can (and should) use internal state to insert delimiters and syntax at the correct places. Example: format->begin_map(format); format->map_key(format, "foo"); format->begin_list(format); format->integer(format, 1); format->integer(format, 2); format->integer(format, 3); format->end(format); format->map_key(format, "bar"); format->begin_map(format); format->map_key(format, "baaz"); format->string(format, "hello world"); format->end(format); format->end(format); would output JSON as follows: {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}} --- sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 sprinter.h diff --git a/sprinter.h b/sprinter.h new file mode 100644 index 0000000..1dad9a0 --- /dev/null +++ b/sprinter.h @@ -0,0 +1,49 @@ +#ifndef NOTMUCH_SPRINTER_H +#define NOTMUCH_SPRINTER_H + +/* for notmuch_bool_t */ +#include "notmuch-client.h" + +/* Structure printer interface */ +typedef struct sprinter +{ + /* start a new map/dictionary structure. + */ + void (*begin_map) (struct sprinter *); + + /* start a new list/array structure + */ + void (*begin_list) (struct sprinter *); + + /* end the last opened list or map structure + */ + void (*end) (struct sprinter *); + + /* print one string/integer/boolean/null element (possibly inside a + * list or map + */ + void (*string) (struct sprinter *, const char *); + void (*integer) (struct sprinter *, int); + void (*boolean) (struct sprinter *, notmuch_bool_t); + void (*null) (struct sprinter *); + + /* print the key of a map's key/value pair. + */ + void (*map_key) (struct sprinter *, const char *); + + /* print a frame delimiter (such as a newline or extra whitespace) + */ + void (*frame) (struct sprinter *); +} sprinter_t; + +/* Create a new structure printer that emits JSON */ +struct sprinter * +sprinter_json_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. + */ +struct sprinter * +sprinter_text; + +#endif // NOTMUCH_SPRINTER_H -- 1.7.11.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven @ 2012-07-12 9:14 ` Mark Walters 2012-07-12 9:21 ` Mark Walters ` (3 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Mark Walters @ 2012-07-12 9:14 UTC (permalink / raw) To: craven, notmuch Hi this definitely looks nice. I have a couple of suggestions for the comments: On Thu, 12 Jul 2012, craven@gmx.net wrote: > This patch adds a new type sprinter_t, which is used for structured > formatting, e.g. JSON or S-Expressions. The structure printer is the > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). > > The structure printer contains the following function pointers: > > /* start a new map/dictionary structure. > */ > void (*begin_map) (struct sprinter *); > > /* start a new list/array structure > */ > void (*begin_list) (struct sprinter *); > > /* end the last opened list or map structure > */ > void (*end) (struct sprinter *); > > /* print one string/integer/boolean/null element (possibly inside a > * list or map > */ > void (*string) (struct sprinter *, const char *); > void (*integer) (struct sprinter *, int); > void (*boolean) (struct sprinter *, notmuch_bool_t); > void (*null) (struct sprinter *); > > /* print the key of a map's key/value pair. > */ > void (*map_key) (struct sprinter *, const char *); > > /* print a frame delimiter (such as a newline or extra whitespace) > */ > void (*frame) (struct sprinter *); > > The printer can (and should) use internal state to insert delimiters and > syntax at the correct places. > > Example: > > format->begin_map(format); > format->map_key(format, "foo"); > format->begin_list(format); > format->integer(format, 1); > format->integer(format, 2); > format->integer(format, 3); > format->end(format); > format->map_key(format, "bar"); > format->begin_map(format); > format->map_key(format, "baaz"); > format->string(format, "hello world"); > format->end(format); > format->end(format); > > would output JSON as follows: > > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}} > --- > sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 sprinter.h > > diff --git a/sprinter.h b/sprinter.h > new file mode 100644 > index 0000000..1dad9a0 > --- /dev/null > +++ b/sprinter.h > @@ -0,0 +1,49 @@ > +#ifndef NOTMUCH_SPRINTER_H > +#define NOTMUCH_SPRINTER_H > + > +/* for notmuch_bool_t */ > +#include "notmuch-client.h" > + > +/* Structure printer interface */ > +typedef struct sprinter > +{ > + /* start a new map/dictionary structure. > + */ > + void (*begin_map) (struct sprinter *); > + > + /* start a new list/array structure > + */ > + void (*begin_list) (struct sprinter *); > + > + /* end the last opened list or map structure > + */ > + void (*end) (struct sprinter *); > + > + /* print one string/integer/boolean/null element (possibly inside a > + * list or map > + */ I think this should say that it also prints any separator necessary. > + void (*string) (struct sprinter *, const char *); > + void (*integer) (struct sprinter *, int); > + void (*boolean) (struct sprinter *, notmuch_bool_t); > + void (*null) (struct sprinter *); > + > + /* print the key of a map's key/value pair. > + */ > + void (*map_key) (struct sprinter *, const char *); > + > + /* print a frame delimiter (such as a newline or extra whitespace) > + */ > + void (*frame) (struct sprinter *); I think this should say that the intention is that this only prints non-syntactic stuff. Best wishes Mark > +} sprinter_t; > + > +/* Create a new structure printer that emits JSON */ > +struct sprinter * > +sprinter_json_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. > + */ > +struct sprinter * > +sprinter_text; > + > +#endif // NOTMUCH_SPRINTER_H > -- > 1.7.11.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven 2012-07-12 9:14 ` Mark Walters @ 2012-07-12 9:21 ` Mark Walters 2012-07-12 11:22 ` craven 2012-07-12 10:05 ` Tomi Ollila ` (2 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Mark Walters @ 2012-07-12 9:21 UTC (permalink / raw) To: craven, notmuch A couple more things: On Thu, 12 Jul 2012, craven@gmx.net wrote: > This patch adds a new type sprinter_t, which is used for structured > formatting, e.g. JSON or S-Expressions. The structure printer is the > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). > > The structure printer contains the following function pointers: > > /* start a new map/dictionary structure. > */ > void (*begin_map) (struct sprinter *); > > /* start a new list/array structure > */ > void (*begin_list) (struct sprinter *); > > /* end the last opened list or map structure > */ > void (*end) (struct sprinter *); 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. 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. Best wishes Mark > /* print one string/integer/boolean/null element (possibly inside a > * list or map > */ > void (*string) (struct sprinter *, const char *); > void (*integer) (struct sprinter *, int); > void (*boolean) (struct sprinter *, notmuch_bool_t); > void (*null) (struct sprinter *); > > /* print the key of a map's key/value pair. > */ > void (*map_key) (struct sprinter *, const char *); > > /* print a frame delimiter (such as a newline or extra whitespace) > */ > void (*frame) (struct sprinter *); > > The printer can (and should) use internal state to insert delimiters and > syntax at the correct places. > > Example: > > format->begin_map(format); > format->map_key(format, "foo"); > format->begin_list(format); > format->integer(format, 1); > format->integer(format, 2); > format->integer(format, 3); > format->end(format); > format->map_key(format, "bar"); > format->begin_map(format); > format->map_key(format, "baaz"); > format->string(format, "hello world"); > format->end(format); > format->end(format); > > would output JSON as follows: > > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}} > --- > sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 sprinter.h > > diff --git a/sprinter.h b/sprinter.h > new file mode 100644 > index 0000000..1dad9a0 > --- /dev/null > +++ b/sprinter.h > @@ -0,0 +1,49 @@ > +#ifndef NOTMUCH_SPRINTER_H > +#define NOTMUCH_SPRINTER_H > + > +/* for notmuch_bool_t */ > +#include "notmuch-client.h" > + > +/* Structure printer interface */ > +typedef struct sprinter > +{ > + /* start a new map/dictionary structure. > + */ > + void (*begin_map) (struct sprinter *); > + > + /* start a new list/array structure > + */ > + void (*begin_list) (struct sprinter *); > + > + /* end the last opened list or map structure > + */ > + void (*end) (struct sprinter *); > + > + /* print one string/integer/boolean/null element (possibly inside a > + * list or map > + */ > + void (*string) (struct sprinter *, const char *); > + void (*integer) (struct sprinter *, int); > + void (*boolean) (struct sprinter *, notmuch_bool_t); > + void (*null) (struct sprinter *); > + > + /* print the key of a map's key/value pair. > + */ > + void (*map_key) (struct sprinter *, const char *); > + > + /* print a frame delimiter (such as a newline or extra whitespace) > + */ > + void (*frame) (struct sprinter *); > +} sprinter_t; > + > +/* Create a new structure printer that emits JSON */ > +struct sprinter * > +sprinter_json_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. > + */ > +struct sprinter * > +sprinter_text; > + > +#endif // NOTMUCH_SPRINTER_H > -- > 1.7.11.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 9:21 ` Mark Walters @ 2012-07-12 11:22 ` craven 2012-07-12 11:25 ` Mark Walters 0 siblings, 1 reply; 18+ messages in thread From: craven @ 2012-07-12 11:22 UTC (permalink / raw) To: Mark Walters, notmuch [-- Attachment #1: Type: text/plain, Size: 874 bytes --] > 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. > 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 :) Thanks for the suggestions! Peter [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-an-S-Expression-output-format.patch --] [-- Type: text/x-patch, Size: 6660 bytes --] From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001 From: <craven@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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 11:22 ` craven @ 2012-07-12 11:25 ` Mark Walters 2012-07-12 12:05 ` Tomi Ollila 0 siblings, 1 reply; 18+ messages in thread From: Mark Walters @ 2012-07-12 11:25 UTC (permalink / raw) To: craven, notmuch On Thu, 12 Jul 2012, craven@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. >> 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. (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 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 > > Thanks for the suggestions! > > Peter > From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001 > From: <craven@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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 11:25 ` Mark Walters @ 2012-07-12 12:05 ` Tomi Ollila 0 siblings, 0 replies; 18+ messages in thread From: Tomi Ollila @ 2012-07-12 12:05 UTC (permalink / raw) To: Mark Walters, craven, notmuch On Thu, Jul 12 2012, Mark Walters <markwalters1009@gmail.com> wrote: > On Thu, 12 Jul 2012, craven@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@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@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven 2012-07-12 9:14 ` Mark Walters 2012-07-12 9:21 ` Mark Walters @ 2012-07-12 10:05 ` Tomi Ollila 2012-07-12 20:29 ` Austin Clements 2012-07-12 22:08 ` Austin Clements 4 siblings, 0 replies; 18+ messages in thread From: Tomi Ollila @ 2012-07-12 10:05 UTC (permalink / raw) To: craven, notmuch On Thu, Jul 12 2012, craven@gmx.net wrote: > This patch adds a new type sprinter_t, which is used for structured > formatting, e.g. JSON or S-Expressions. The structure printer is the > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). ... > + > +/* Create a new structure printer that emits JSON */ > +struct sprinter * > +sprinter_json_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. > + */ > +struct sprinter * > +sprinter_text; Looks good... but this needs to be 'extern struct sprinter * sprinter_text;' see id:"1340563714-3103-1-git-send-email-tomi.ollila@iki.fi" for reference. > + > +#endif // NOTMUCH_SPRINTER_H > -- > 1.7.11.1 Tomi ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven ` (2 preceding siblings ...) 2012-07-12 10:05 ` Tomi Ollila @ 2012-07-12 20:29 ` Austin Clements 2012-07-12 22:08 ` Austin Clements 4 siblings, 0 replies; 18+ messages in thread From: Austin Clements @ 2012-07-12 20:29 UTC (permalink / raw) To: craven; +Cc: notmuch Quoth craven@gmx.net on Jul 12 at 9:43 am: > This patch adds a new type sprinter_t, which is used for structured > formatting, e.g. JSON or S-Expressions. The structure printer is the > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). > > The structure printer contains the following function pointers: > > /* start a new map/dictionary structure. > */ > void (*begin_map) (struct sprinter *); > > /* start a new list/array structure > */ > void (*begin_list) (struct sprinter *); > > /* end the last opened list or map structure > */ > void (*end) (struct sprinter *); > > /* print one string/integer/boolean/null element (possibly inside a > * list or map > */ > void (*string) (struct sprinter *, const char *); > void (*integer) (struct sprinter *, int); > void (*boolean) (struct sprinter *, notmuch_bool_t); > void (*null) (struct sprinter *); > > /* print the key of a map's key/value pair. > */ > void (*map_key) (struct sprinter *, const char *); > > /* print a frame delimiter (such as a newline or extra whitespace) > */ > void (*frame) (struct sprinter *); > > The printer can (and should) use internal state to insert delimiters and > syntax at the correct places. > > Example: > > format->begin_map(format); > format->map_key(format, "foo"); > format->begin_list(format); > format->integer(format, 1); > format->integer(format, 2); > format->integer(format, 3); > format->end(format); > format->map_key(format, "bar"); > format->begin_map(format); > format->map_key(format, "baaz"); > format->string(format, "hello world"); > format->end(format); > format->end(format); > > would output JSON as follows: > > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}} > --- > sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 sprinter.h > > diff --git a/sprinter.h b/sprinter.h > new file mode 100644 > index 0000000..1dad9a0 > --- /dev/null > +++ b/sprinter.h > @@ -0,0 +1,49 @@ > +#ifndef NOTMUCH_SPRINTER_H > +#define NOTMUCH_SPRINTER_H > + > +/* for notmuch_bool_t */ Style/consistency nit: all of the comments should start with a capital letter and anything that's a sentence should end with a period (some of your comments do, some of them don't). > +#include "notmuch-client.h" > + > +/* Structure printer interface */ > +typedef struct sprinter > +{ > + /* start a new map/dictionary structure. This should probably mention how other functions should be called within the map structure. Perhaps, /* Start a new map/dictionary structure. This should be followed by a * sequence of alternating calls to map_key and one of the value * printing functions until the map is ended. */ (You had a comment to this effect in one of your earlier patches.) > + */ > + void (*begin_map) (struct sprinter *); > + > + /* start a new list/array structure > + */ > + void (*begin_list) (struct sprinter *); > + > + /* end the last opened list or map structure > + */ > + void (*end) (struct sprinter *); > + > + /* print one string/integer/boolean/null element (possibly inside a > + * list or map Missing close paren. > + */ > + void (*string) (struct sprinter *, const char *); > + void (*integer) (struct sprinter *, int); > + void (*boolean) (struct sprinter *, notmuch_bool_t); > + void (*null) (struct sprinter *); > + > + /* print the key of a map's key/value pair. > + */ > + void (*map_key) (struct sprinter *, const char *); > + > + /* print a frame delimiter (such as a newline or extra whitespace) > + */ > + void (*frame) (struct sprinter *); I wonder if frame is still necessary. At the time, I was thinking that we would use embedded newline framing to do streaming JSON parsing, but I wound up going with a general incremental JSON parser, eliminating the need for framing. It's probably still useful to have a function that inserts a newline purely for readability/debuggability purposes. In practice, the implementation would be the same as frame (though if it's for readability, maybe you'd want to print the comma before the newline instead of after), but since the intent would be different, it would need different documentation and maybe a different name. "Frame" isn't a bad name for either intent. We can't use "break". "Separator" (or just "sep")? "Space"? "Split"? The comment could be something like /* Insert a separator for improved readability without affecting the * abstract syntax of the structure being printed. For JSON, this is * simply a line break. */ > +} sprinter_t; > + > +/* Create a new structure printer that emits JSON */ > +struct sprinter * > +sprinter_json_new(const void *ctx, FILE *stream); This should probably be called sprinter_json_create to be consistent with the naming of other constructors in notmuch. Also, missing space between the function name and parameter list (sorry, my fault). > + > +/* A dummy structure printer that signifies that standard text output is > + * to be used instead of any structured format. > + */ > +struct sprinter * > +sprinter_text; It looks like you never assign anything to this pointer, so all of your tests against sprinter_text are equivalent to just checking for a NULL pointer. You could either just use NULL as the designated value for the text format, or you could use this global variable, but make it a struct sprinter instead of a struct sprinter *. Also, this can probably be a static declaration in notmuch-search.c. > + > +#endif // NOTMUCH_SPRINTER_H ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] Add support for structured output formatters. 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven ` (3 preceding siblings ...) 2012-07-12 20:29 ` Austin Clements @ 2012-07-12 22:08 ` Austin Clements 4 siblings, 0 replies; 18+ messages in thread From: Austin Clements @ 2012-07-12 22:08 UTC (permalink / raw) To: craven; +Cc: notmuch Quoth craven@gmx.net on Jul 12 at 9:43 am: > This patch adds a new type sprinter_t, which is used for structured > formatting, e.g. JSON or S-Expressions. The structure printer is the > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu). > > The structure printer contains the following function pointers: > > /* start a new map/dictionary structure. > */ > void (*begin_map) (struct sprinter *); > > /* start a new list/array structure > */ > void (*begin_list) (struct sprinter *); > > /* end the last opened list or map structure > */ > void (*end) (struct sprinter *); > > /* print one string/integer/boolean/null element (possibly inside a > * list or map > */ > void (*string) (struct sprinter *, const char *); > void (*integer) (struct sprinter *, int); > void (*boolean) (struct sprinter *, notmuch_bool_t); > void (*null) (struct sprinter *); > > /* print the key of a map's key/value pair. > */ > void (*map_key) (struct sprinter *, const char *); > > /* print a frame delimiter (such as a newline or extra whitespace) > */ > void (*frame) (struct sprinter *); > > The printer can (and should) use internal state to insert delimiters and > syntax at the correct places. > > Example: > > format->begin_map(format); > format->map_key(format, "foo"); > format->begin_list(format); > format->integer(format, 1); > format->integer(format, 2); > format->integer(format, 3); > format->end(format); > format->map_key(format, "bar"); > format->begin_map(format); > format->map_key(format, "baaz"); > format->string(format, "hello world"); > format->end(format); > format->end(format); > > would output JSON as follows: > > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}} > --- > sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 sprinter.h > > diff --git a/sprinter.h b/sprinter.h > new file mode 100644 > index 0000000..1dad9a0 > --- /dev/null > +++ b/sprinter.h > @@ -0,0 +1,49 @@ > +#ifndef NOTMUCH_SPRINTER_H > +#define NOTMUCH_SPRINTER_H > + > +/* for notmuch_bool_t */ > +#include "notmuch-client.h" > + > +/* Structure printer interface */ > +typedef struct sprinter > +{ > + /* start a new map/dictionary structure. > + */ > + void (*begin_map) (struct sprinter *); > + > + /* start a new list/array structure > + */ > + void (*begin_list) (struct sprinter *); > + > + /* end the last opened list or map structure > + */ > + void (*end) (struct sprinter *); > + > + /* print one string/integer/boolean/null element (possibly inside a > + * list or map > + */ > + void (*string) (struct sprinter *, const char *); Oh, also, the documentation for string should mention the string argument should be UTF-8 encoded. > + void (*integer) (struct sprinter *, int); > + void (*boolean) (struct sprinter *, notmuch_bool_t); > + void (*null) (struct sprinter *); > + > + /* print the key of a map's key/value pair. > + */ > + void (*map_key) (struct sprinter *, const char *); > + > + /* print a frame delimiter (such as a newline or extra whitespace) > + */ > + void (*frame) (struct sprinter *); > +} sprinter_t; > + > +/* Create a new structure printer that emits JSON */ > +struct sprinter * > +sprinter_json_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. > + */ > +struct sprinter * > +sprinter_text; > + > +#endif // NOTMUCH_SPRINTER_H ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 2/3] Add structured output formatter for JSON. 2012-07-12 7:43 ` Structured Formatters for JSON Output craven 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven @ 2012-07-12 7:43 ` craven 2012-07-12 10:10 ` Tomi Ollila 2012-07-12 22:07 ` Austin Clements 2012-07-12 7:43 ` [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search craven 2 siblings, 2 replies; 18+ messages in thread From: craven @ 2012-07-12 7:43 UTC (permalink / raw) To: notmuch Using the new structured printer support in sprinter.h, implement sprinter_json_new, which returns a new JSON structured output formatter. The formatter prints output similar to the existing JSON, but with differences in whitespace (mostly newlines). --- Makefile.local | 1 + sprinter.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) create mode 100644 sprinter.c diff --git a/Makefile.local b/Makefile.local index a890df2..8baf0c2 100644 --- a/Makefile.local +++ b/Makefile.local @@ -290,6 +290,7 @@ notmuch_client_srcs = \ notmuch-show.c \ notmuch-tag.c \ notmuch-time.c \ + sprinter.c \ query-string.c \ mime-node.c \ crypto.c \ diff --git a/sprinter.c b/sprinter.c new file mode 100644 index 0000000..649f79a --- /dev/null +++ b/sprinter.c @@ -0,0 +1,172 @@ +#include <stdbool.h> +#include <stdio.h> +#include <talloc.h> +#include "sprinter.h" + +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0])) + +struct sprinter * +sprinter_text = NULL; + +/* + * Every below here is the implementation of the JSON printer. + */ + +struct sprinter_json +{ + struct sprinter vtable; + FILE *stream; + /* Top of the state stack, or NULL if the printer is not currently + * inside any aggregate types. */ + struct json_state *state; +}; + +struct json_state +{ + struct json_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. */ + char close; +}; + +/* Helper function to set up the stream to print a value. If this + * value follows another value, prints a comma. */ +static struct sprinter_json * +json_begin_value(struct sprinter *sp) +{ + struct sprinter_json *spj = (struct sprinter_json*)sp; + if (spj->state) { + if (!spj->state->first) + fputs (", ", spj->stream); + else + spj->state->first = false; + } + return spj; +} + +/* Helper function to begin an aggregate type. Prints the open + * character and pushes a new state frame. */ +static void +json_begin_aggregate(struct sprinter *sp, char open, char close) +{ + struct sprinter_json *spj = json_begin_value (sp); + struct json_state *state = talloc (spj, struct json_state); + + fputc (open, spj->stream); + state->parent = spj->state; + state->first = true; + state->close = close; + spj->state = state; +} + +static void +json_begin_map(struct sprinter *sp) +{ + json_begin_aggregate (sp, '{', '}'); +} + +static void +json_begin_list(struct sprinter *sp) +{ + json_begin_aggregate (sp, '[', ']'); +} + +static void +json_end(struct sprinter *sp) +{ + struct sprinter_json *spj = (struct sprinter_json*)sp; + struct json_state *state = spj->state; + + fputc (spj->state->close, spj->stream); + spj->state = state->parent; + talloc_free (state); + if(spj->state == NULL) + fputc ('\n', spj->stream); +} + +static void +json_string(struct sprinter *sp, const char *val) +{ + static const char * const escapes[] = { + ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b", + ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t" + }; + struct sprinter_json *spj = json_begin_value (sp); + fputc ('"', spj->stream); + for (; *val; ++val) { + unsigned char ch = *val; + if (ch < ARRAY_SIZE(escapes) && escapes[ch]) + fputs (escapes[ch], spj->stream); + else if (ch >= 32) + fputc (ch, spj->stream); + else + fprintf (spj->stream, "\\u%04x", ch); + } + fputc ('"', spj->stream); +} + +static void +json_integer(struct sprinter *sp, int val) +{ + struct sprinter_json *spj = json_begin_value (sp); + fprintf (spj->stream, "%d", val); +} + +static void +json_boolean(struct sprinter *sp, notmuch_bool_t val) +{ + struct sprinter_json *spj = json_begin_value (sp); + fputs (val ? "true" : "false", spj->stream); +} + +static void +json_null(struct sprinter *sp) +{ + struct sprinter_json *spj = json_begin_value (sp); + fputs ("null", spj->stream); +} + +static void +json_map_key(struct sprinter *sp, const char *key) +{ + struct sprinter_json *spj = (struct sprinter_json*)sp; + json_string (sp, key); + fputs (": ", spj->stream); + spj->state->first = true; +} + +static void +json_frame(struct sprinter *sp) +{ + struct sprinter_json *spj = (struct sprinter_json*)sp; + fputc ('\n', spj->stream); +} + +struct sprinter * +sprinter_json_new(const void *ctx, FILE *stream) +{ + static const struct sprinter_json template = { + .vtable = { + .begin_map = json_begin_map, + .begin_list = json_begin_list, + .end = json_end, + .string = json_string, + .integer = json_integer, + .boolean = json_boolean, + .null = json_null, + .map_key = json_map_key, + .frame = json_frame, + } + }; + struct sprinter_json *res; + + res = talloc (ctx, struct sprinter_json); + if (!res) + return NULL; + + *res = template; + res->stream = stream; + return &res->vtable; +} -- 1.7.11.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] Add structured output formatter for JSON. 2012-07-12 7:43 ` [PATCH v4 2/3] Add structured output formatter for JSON craven @ 2012-07-12 10:10 ` Tomi Ollila 2012-07-12 20:34 ` Austin Clements 2012-07-12 22:07 ` Austin Clements 1 sibling, 1 reply; 18+ messages in thread From: Tomi Ollila @ 2012-07-12 10:10 UTC (permalink / raw) To: craven, notmuch On Thu, Jul 12 2012, craven@gmx.net wrote: > Using the new structured printer support in sprinter.h, implement > sprinter_json_new, which returns a new JSON structured output > formatter. > > The formatter prints output similar to the existing JSON, but with > differences in whitespace (mostly newlines). > --- > Makefile.local | 1 + > sprinter.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 173 insertions(+) > create mode 100644 sprinter.c > > diff --git a/Makefile.local b/Makefile.local > index a890df2..8baf0c2 100644 > --- a/Makefile.local > +++ b/Makefile.local > @@ -290,6 +290,7 @@ notmuch_client_srcs = \ > notmuch-show.c \ > notmuch-tag.c \ > notmuch-time.c \ > + sprinter.c \ > query-string.c \ > mime-node.c \ > crypto.c \ > diff --git a/sprinter.c b/sprinter.c > new file mode 100644 > index 0000000..649f79a > --- /dev/null > +++ b/sprinter.c > @@ -0,0 +1,172 @@ > +#include <stdbool.h> > +#include <stdio.h> > +#include <talloc.h> > +#include "sprinter.h" > + > +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0])) You're including sprinter.h which includes notmuch-client.h which defines ARRAY_SIZE (Interesting that you did not get error(/warning?) about this) Rest looks good -- except the whitespace -- as I looked through Austin's code yesterday you're just replicated the same lines :D. Easiest to fix is probably just running uncrustify -c devel/uncrustify.cfg --replace sprinter.c Tomi > + > +struct sprinter * > +sprinter_text = NULL; > + > +/* > + * Every below here is the implementation of the JSON printer. > + */ > + > +struct sprinter_json > +{ > + struct sprinter vtable; > + FILE *stream; > + /* Top of the state stack, or NULL if the printer is not currently > + * inside any aggregate types. */ > + struct json_state *state; > +}; > + > +struct json_state > +{ > + struct json_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. */ > + char close; > +}; > + > +/* Helper function to set up the stream to print a value. If this > + * value follows another value, prints a comma. */ > +static struct sprinter_json * > +json_begin_value(struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + if (spj->state) { > + if (!spj->state->first) > + fputs (", ", spj->stream); > + else > + spj->state->first = false; > + } > + return spj; > +} > + > +/* Helper function to begin an aggregate type. Prints the open > + * character and pushes a new state frame. */ > +static void > +json_begin_aggregate(struct sprinter *sp, char open, char close) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + struct json_state *state = talloc (spj, struct json_state); > + > + fputc (open, spj->stream); > + state->parent = spj->state; > + state->first = true; > + state->close = close; > + spj->state = state; > +} > + > +static void > +json_begin_map(struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '{', '}'); > +} > + > +static void > +json_begin_list(struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '[', ']'); > +} > + > +static void > +json_end(struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + struct json_state *state = spj->state; > + > + fputc (spj->state->close, spj->stream); > + spj->state = state->parent; > + talloc_free (state); > + if(spj->state == NULL) > + fputc ('\n', spj->stream); > +} > + > +static void > +json_string(struct sprinter *sp, const char *val) > +{ > + static const char * const escapes[] = { > + ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b", > + ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t" > + }; > + struct sprinter_json *spj = json_begin_value (sp); > + fputc ('"', spj->stream); > + for (; *val; ++val) { > + unsigned char ch = *val; > + if (ch < ARRAY_SIZE(escapes) && escapes[ch]) > + fputs (escapes[ch], spj->stream); > + else if (ch >= 32) > + fputc (ch, spj->stream); > + else > + fprintf (spj->stream, "\\u%04x", ch); > + } > + fputc ('"', spj->stream); > +} > + > +static void > +json_integer(struct sprinter *sp, int val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fprintf (spj->stream, "%d", val); > +} > + > +static void > +json_boolean(struct sprinter *sp, notmuch_bool_t val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fputs (val ? "true" : "false", spj->stream); > +} > + > +static void > +json_null(struct sprinter *sp) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fputs ("null", spj->stream); > +} > + > +static void > +json_map_key(struct sprinter *sp, const char *key) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + json_string (sp, key); > + fputs (": ", spj->stream); > + spj->state->first = true; > +} > + > +static void > +json_frame(struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + fputc ('\n', spj->stream); > +} > + > +struct sprinter * > +sprinter_json_new(const void *ctx, FILE *stream) > +{ > + static const struct sprinter_json template = { > + .vtable = { > + .begin_map = json_begin_map, > + .begin_list = json_begin_list, > + .end = json_end, > + .string = json_string, > + .integer = json_integer, > + .boolean = json_boolean, > + .null = json_null, > + .map_key = json_map_key, > + .frame = json_frame, > + } > + }; > + struct sprinter_json *res; > + > + res = talloc (ctx, struct sprinter_json); > + if (!res) > + return NULL; > + > + *res = template; > + res->stream = stream; > + return &res->vtable; > +} > -- > 1.7.11.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] Add structured output formatter for JSON. 2012-07-12 10:10 ` Tomi Ollila @ 2012-07-12 20:34 ` Austin Clements 0 siblings, 0 replies; 18+ messages in thread From: Austin Clements @ 2012-07-12 20:34 UTC (permalink / raw) To: Tomi Ollila; +Cc: notmuch Quoth Tomi Ollila on Jul 12 at 1:10 pm: > On Thu, Jul 12 2012, craven@gmx.net wrote: > > > Using the new structured printer support in sprinter.h, implement > > sprinter_json_new, which returns a new JSON structured output > > formatter. > > > > The formatter prints output similar to the existing JSON, but with > > differences in whitespace (mostly newlines). > > --- > > Makefile.local | 1 + > > sprinter.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 173 insertions(+) > > create mode 100644 sprinter.c > > > > diff --git a/Makefile.local b/Makefile.local > > index a890df2..8baf0c2 100644 > > --- a/Makefile.local > > +++ b/Makefile.local > > @@ -290,6 +290,7 @@ notmuch_client_srcs = \ > > notmuch-show.c \ > > notmuch-tag.c \ > > notmuch-time.c \ > > + sprinter.c \ > > query-string.c \ > > mime-node.c \ > > crypto.c \ > > diff --git a/sprinter.c b/sprinter.c > > new file mode 100644 > > index 0000000..649f79a > > --- /dev/null > > +++ b/sprinter.c > > @@ -0,0 +1,172 @@ > > +#include <stdbool.h> > > +#include <stdio.h> > > +#include <talloc.h> > > +#include "sprinter.h" > > + > > +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0])) > > You're including sprinter.h which includes notmuch-client.h which > defines ARRAY_SIZE (Interesting that you did not get error(/warning?) > about this) > > Rest looks good -- except the whitespace -- as I looked through Austin's > code yesterday you're just replicated the same lines :D. Easiest > to fix is probably just running > uncrustify -c devel/uncrustify.cfg --replace sprinter.c Oops. Shame on me. Is anything wrong besides the missing space before the argument list of all of the function declarations? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] Add structured output formatter for JSON. 2012-07-12 7:43 ` [PATCH v4 2/3] Add structured output formatter for JSON craven 2012-07-12 10:10 ` Tomi Ollila @ 2012-07-12 22:07 ` Austin Clements 1 sibling, 0 replies; 18+ messages in thread From: Austin Clements @ 2012-07-12 22:07 UTC (permalink / raw) To: craven; +Cc: notmuch Quoth craven@gmx.net on Jul 12 at 9:43 am: > Using the new structured printer support in sprinter.h, implement > sprinter_json_new, which returns a new JSON structured output > formatter. > > The formatter prints output similar to the existing JSON, but with > differences in whitespace (mostly newlines). > --- > Makefile.local | 1 + > sprinter.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 173 insertions(+) > create mode 100644 sprinter.c > > diff --git a/Makefile.local b/Makefile.local > index a890df2..8baf0c2 100644 > --- a/Makefile.local > +++ b/Makefile.local > @@ -290,6 +290,7 @@ notmuch_client_srcs = \ > notmuch-show.c \ > notmuch-tag.c \ > notmuch-time.c \ > + sprinter.c \ > query-string.c \ > mime-node.c \ > crypto.c \ > diff --git a/sprinter.c b/sprinter.c > new file mode 100644 > index 0000000..649f79a > --- /dev/null > +++ b/sprinter.c > @@ -0,0 +1,172 @@ > +#include <stdbool.h> > +#include <stdio.h> > +#include <talloc.h> > +#include "sprinter.h" > + > +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0])) > + > +struct sprinter * > +sprinter_text = NULL; > + > +/* > + * Every below here is the implementation of the JSON printer. s/Every/Everything/ Or this can be shortened to just /* * JSON printer */ If we wind up with separate files for the JSON and S-exp printers, I don't think this comment is necessary at all. > + */ > + > +struct sprinter_json > +{ > + struct sprinter vtable; > + FILE *stream; > + /* Top of the state stack, or NULL if the printer is not currently > + * inside any aggregate types. */ > + struct json_state *state; > +}; > + > +struct json_state > +{ > + struct json_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. */ > + char close; > +}; > + > +/* Helper function to set up the stream to print a value. If this > + * value follows another value, prints a comma. */ > +static struct sprinter_json * > +json_begin_value(struct sprinter *sp) As Tomi pointed out, there should be spaces before the parameter lists (sorry). > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + if (spj->state) { > + if (!spj->state->first) > + fputs (", ", spj->stream); > + else > + spj->state->first = false; > + } > + return spj; > +} > + > +/* Helper function to begin an aggregate type. Prints the open > + * character and pushes a new state frame. */ > +static void > +json_begin_aggregate(struct sprinter *sp, char open, char close) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + struct json_state *state = talloc (spj, struct json_state); > + > + fputc (open, spj->stream); > + state->parent = spj->state; > + state->first = true; > + state->close = close; > + spj->state = state; > +} > + > +static void > +json_begin_map(struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '{', '}'); > +} > + > +static void > +json_begin_list(struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '[', ']'); > +} > + > +static void > +json_end(struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + struct json_state *state = spj->state; > + > + fputc (spj->state->close, spj->stream); > + spj->state = state->parent; > + talloc_free (state); > + if(spj->state == NULL) Ah, another missing space. > + fputc ('\n', spj->stream); > +} > + > +static void > +json_string(struct sprinter *sp, const char *val) > +{ > + static const char * const escapes[] = { > + ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b", > + ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t" > + }; > + struct sprinter_json *spj = json_begin_value (sp); > + fputc ('"', spj->stream); > + for (; *val; ++val) { > + unsigned char ch = *val; > + if (ch < ARRAY_SIZE(escapes) && escapes[ch]) > + fputs (escapes[ch], spj->stream); > + else if (ch >= 32) > + fputc (ch, spj->stream); > + else > + fprintf (spj->stream, "\\u%04x", ch); > + } > + fputc ('"', spj->stream); > +} > + > +static void > +json_integer(struct sprinter *sp, int val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fprintf (spj->stream, "%d", val); > +} > + > +static void > +json_boolean(struct sprinter *sp, notmuch_bool_t val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fputs (val ? "true" : "false", spj->stream); > +} > + > +static void > +json_null(struct sprinter *sp) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + fputs ("null", spj->stream); > +} > + > +static void > +json_map_key(struct sprinter *sp, const char *key) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + json_string (sp, key); > + fputs (": ", spj->stream); > + spj->state->first = true; > +} > + > +static void > +json_frame(struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json*)sp; > + fputc ('\n', spj->stream); > +} > + > +struct sprinter * > +sprinter_json_new(const void *ctx, FILE *stream) > +{ > + static const struct sprinter_json template = { > + .vtable = { > + .begin_map = json_begin_map, > + .begin_list = json_begin_list, > + .end = json_end, > + .string = json_string, > + .integer = json_integer, > + .boolean = json_boolean, > + .null = json_null, > + .map_key = json_map_key, > + .frame = json_frame, > + } > + }; > + struct sprinter_json *res; > + > + res = talloc (ctx, struct sprinter_json); > + if (!res) > + return NULL; > + > + *res = template; > + res->stream = stream; > + return &res->vtable; > +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search. 2012-07-12 7:43 ` Structured Formatters for JSON Output craven 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven 2012-07-12 7:43 ` [PATCH v4 2/3] Add structured output formatter for JSON craven @ 2012-07-12 7:43 ` craven 2012-07-13 0:02 ` Austin Clements 2 siblings, 1 reply; 18+ messages in thread From: craven @ 2012-07-12 7:43 UTC (permalink / raw) To: notmuch This patch switches from the current ad-hoc printer to the structured output formatter in sprinter.h. It removes search_format_t, replaces it by sprinter_t and inlines the text printer where necessary. The tests are changed (only whitespaces and regular expressions) in order to make them PASS for the new structured output formatter. --- notmuch-search.c | 349 ++++++++++++++++++++++------------------------------- test/json | 20 +-- test/search-output | 270 +++++++++++++++++++++-------------------- 3 files changed, 288 insertions(+), 351 deletions(-) diff --git a/notmuch-search.c b/notmuch-search.c index 3be296d..b853f5f 100644 --- a/notmuch-search.c +++ b/notmuch-search.c @@ -19,6 +19,7 @@ */ #include "notmuch-client.h" +#include "sprinter.h" typedef enum { OUTPUT_SUMMARY, @@ -28,91 +29,9 @@ typedef enum { OUTPUT_TAGS } output_t; -typedef struct search_format { - const char *results_start; - const char *item_start; - void (*item_id) (const void *ctx, - const char *item_type, - const char *item_id); - void (*thread_summary) (const void *ctx, - const char *thread_id, - const time_t date, - const int matched, - const int total, - const char *authors, - const char *subject); - const char *tag_start; - const char *tag; - const char *tag_sep; - const char *tag_end; - const char *item_sep; - const char *item_end; - const char *results_end; - const char *results_null; -} search_format_t; - -static void -format_item_id_text (const void *ctx, - const char *item_type, - const char *item_id); - -static void -format_thread_text (const void *ctx, - const char *thread_id, - const time_t date, - const int matched, - const int total, - const char *authors, - const char *subject); -static const search_format_t format_text = { - "", - "", - format_item_id_text, - format_thread_text, - " (", - "%s", " ", - ")", "\n", - "", - "\n", - "", -}; - -static void -format_item_id_json (const void *ctx, - const char *item_type, - const char *item_id); - -static void -format_thread_json (const void *ctx, - const char *thread_id, - const time_t date, - const int matched, - const int total, - const char *authors, - const char *subject); - -/* Any changes to the JSON format should be reflected in the file - * devel/schemata. */ -static const search_format_t format_json = { - "[", - "{", - format_item_id_json, - format_thread_json, - "\"tags\": [", - "\"%s\"", ", ", - "]", ",\n", - "}", - "]\n", - "]\n", -}; - -static void -format_item_id_text (unused (const void *ctx), - const char *item_type, - const char *item_id) -{ - printf ("%s%s", item_type, item_id); -} +static const char * text_item_sep = "\n"; +static const char * text_results_null = ""; +static const char * text_results_end = "\n"; static char * sanitize_string (const void *ctx, const char *str) @@ -131,72 +50,8 @@ sanitize_string (const void *ctx, const char *str) return out; } -static void -format_thread_text (const void *ctx, - const char *thread_id, - const time_t date, - const int matched, - const int total, - const char *authors, - const char *subject) -{ - void *ctx_quote = talloc_new (ctx); - - printf ("thread:%s %12s [%d/%d] %s; %s", - thread_id, - notmuch_time_relative_date (ctx, date), - matched, - total, - sanitize_string (ctx_quote, authors), - sanitize_string (ctx_quote, subject)); - - talloc_free (ctx_quote); -} - -static void -format_item_id_json (const void *ctx, - unused (const char *item_type), - const char *item_id) -{ - void *ctx_quote = talloc_new (ctx); - - printf ("%s", json_quote_str (ctx_quote, item_id)); - - talloc_free (ctx_quote); - -} - -static void -format_thread_json (const void *ctx, - const char *thread_id, - const time_t date, - const int matched, - const int total, - const char *authors, - const char *subject) -{ - void *ctx_quote = talloc_new (ctx); - - printf ("\"thread\": %s,\n" - "\"timestamp\": %ld,\n" - "\"date_relative\": \"%s\",\n" - "\"matched\": %d,\n" - "\"total\": %d,\n" - "\"authors\": %s,\n" - "\"subject\": %s,\n", - json_quote_str (ctx_quote, thread_id), - date, - notmuch_time_relative_date (ctx, date), - matched, - total, - json_quote_str (ctx_quote, authors), - json_quote_str (ctx_quote, subject)); - - talloc_free (ctx_quote); -} - static int -do_search_threads (const search_format_t *format, +do_search_threads (sprinter_t *format, notmuch_query_t *query, notmuch_sort_t sort, output_t output, @@ -220,7 +75,9 @@ do_search_threads (const search_format_t *format, if (threads == NULL) return 1; - fputs (format->results_start, stdout); + if (format != sprinter_text) { + format->begin_list (format); + } for (i = 0; notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit); @@ -235,43 +92,96 @@ do_search_threads (const search_format_t *format, continue; } - if (! first_thread) - fputs (format->item_sep, stdout); + if (format == sprinter_text && ! first_thread) + fputs (text_item_sep, stdout); if (output == OUTPUT_THREADS) { - format->item_id (thread, "thread:", - notmuch_thread_get_thread_id (thread)); + const char *thread_id = notmuch_thread_get_thread_id (thread); + if (format == sprinter_text) + printf ("thread:%s", thread_id); + else { + format->string (format, thread_id); + format->frame (format); + } + } else { /* output == OUTPUT_SUMMARY */ - fputs (format->item_start, stdout); + const char *authors = notmuch_thread_get_authors (thread); + const char *subject = notmuch_thread_get_subject (thread); + const char *thread_id = notmuch_thread_get_thread_id (thread); + int matched = notmuch_thread_get_matched_messages (thread); + int total = notmuch_thread_get_total_messages (thread); + const char *relative_date = NULL; + + if (format != sprinter_text) + format->begin_map (format); + if (sort == NOTMUCH_SORT_OLDEST_FIRST) date = notmuch_thread_get_oldest_date (thread); else date = notmuch_thread_get_newest_date (thread); - format->thread_summary (thread, - notmuch_thread_get_thread_id (thread), - date, - notmuch_thread_get_matched_messages (thread), - notmuch_thread_get_total_messages (thread), - notmuch_thread_get_authors (thread), - notmuch_thread_get_subject (thread)); + void *ctx_quote = talloc_new (thread); + relative_date = + notmuch_time_relative_date (ctx_quote, date); + + if (format == sprinter_text) { + printf ("thread:%s %12s [%d/%d] %s; %s", + thread_id, + relative_date, + matched, + total, + sanitize_string (ctx_quote, authors), + sanitize_string (ctx_quote, subject)); + } else { + format->map_key (format, "thread"); + format->string (format, thread_id); + format->map_key (format, "timestamp"); + format->integer (format, date); + format->map_key (format, "date_relative"); + format->string (format, relative_date); + format->map_key (format, "matched"); + format->integer (format, matched); + format->map_key (format, "total"); + format->integer (format, total); + format->map_key (format, "authors"); + format->string (format, authors); + format->map_key (format, "subject"); + format->string (format, subject); + } + + talloc_free (ctx_quote); - fputs (format->tag_start, stdout); + if (format == sprinter_text) { + fputs (" (", stdout); + } else { + format->map_key (format, "tags"); + format->begin_list (format); + } for (tags = notmuch_thread_get_tags (thread); notmuch_tags_valid (tags); notmuch_tags_move_to_next (tags)) { - if (! first_tag) - fputs (format->tag_sep, stdout); - printf (format->tag, notmuch_tags_get (tags)); + const char *tag = notmuch_tags_get (tags); + if (format == sprinter_text) { + if (! first_tag) + fputs (" ", stdout); + fputs (tag, stdout); + } else { + format->string (format, tag); + } + first_tag = 0; } - fputs (format->tag_end, stdout); - - fputs (format->item_end, stdout); + if (format == sprinter_text) { + fputs (")", stdout); + } else { + format->end (format); + format->end (format); + format->frame (format); + } } first_thread = 0; @@ -279,16 +189,20 @@ do_search_threads (const search_format_t *format, notmuch_thread_destroy (thread); } - if (first_thread) - fputs (format->results_null, stdout); - else - fputs (format->results_end, stdout); + if (format == sprinter_text) + if (first_thread) + fputs (text_results_null, stdout); + else + fputs (text_results_end, stdout); + else { + format->end (format); + } return 0; } static int -do_search_messages (const search_format_t *format, +do_search_messages (sprinter_t *format, notmuch_query_t *query, output_t output, int offset, @@ -310,7 +224,8 @@ do_search_messages (const search_format_t *format, if (messages == NULL) return 1; - fputs (format->results_start, stdout); + if (format != sprinter_text) + format->begin_list (format); for (i = 0; notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit); @@ -328,23 +243,36 @@ do_search_messages (const search_format_t *format, notmuch_filenames_valid (filenames); notmuch_filenames_move_to_next (filenames)) { - if (! first_message) - fputs (format->item_sep, stdout); + const char *filenames_str = notmuch_filenames_get (filenames); + + if (format == sprinter_text && ! first_message) + fputs (text_item_sep, stdout); - format->item_id (message, "", - notmuch_filenames_get (filenames)); + if (format == sprinter_text) + fputs (filenames_str, stdout); + else { + format->string (format, filenames_str); + format->frame (format); + } first_message = 0; } - - notmuch_filenames_destroy( filenames ); + + notmuch_filenames_destroy (filenames); } else { /* output == OUTPUT_MESSAGES */ - if (! first_message) - fputs (format->item_sep, stdout); + const char *message_id = notmuch_message_get_message_id (message); + + if (format == sprinter_text && ! first_message) + fputs (text_item_sep, stdout); + + if (format == sprinter_text) + printf ("id:%s", message_id); + else { + format->string (format, message_id); + format->frame (format); + } - format->item_id (message, "id:", - notmuch_message_get_message_id (message)); first_message = 0; } @@ -353,17 +281,21 @@ do_search_messages (const search_format_t *format, notmuch_messages_destroy (messages); - if (first_message) - fputs (format->results_null, stdout); - else - fputs (format->results_end, stdout); + if (format == sprinter_text) + if (first_message) + fputs (text_results_null, stdout); + else + fputs (text_results_end, stdout); + else { + format->end (format); + } return 0; } static int do_search_tags (notmuch_database_t *notmuch, - const search_format_t *format, + sprinter_t *format, notmuch_query_t *query) { notmuch_messages_t *messages = NULL; @@ -387,7 +319,8 @@ do_search_tags (notmuch_database_t *notmuch, if (tags == NULL) return 1; - fputs (format->results_start, stdout); + if (format != sprinter_text) + format->begin_list (format); for (; notmuch_tags_valid (tags); @@ -395,10 +328,15 @@ do_search_tags (notmuch_database_t *notmuch, { tag = notmuch_tags_get (tags); - if (! first_tag) - fputs (format->item_sep, stdout); + if (format == sprinter_text && ! first_tag) + fputs (text_item_sep, stdout); - format->item_id (tags, "", tag); + if (format == sprinter_text) + fputs (tag, stdout); + else { + format->string (format, tag); + format->frame (format); + } first_tag = 0; } @@ -408,10 +346,14 @@ do_search_tags (notmuch_database_t *notmuch, if (messages) notmuch_messages_destroy (messages); - if (first_tag) - fputs (format->results_null, stdout); - else - fputs (format->results_end, stdout); + if (format == sprinter_text) + if (first_tag) + fputs (text_results_null, stdout); + else + fputs (text_results_end, stdout); + else { + format->end (format); + } return 0; } @@ -430,7 +372,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) notmuch_query_t *query; char *query_str; notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST; - const search_format_t *format = &format_text; + sprinter_t *format = NULL; /* the default output is text */ int opt_index, ret; output_t output = OUTPUT_SUMMARY; int offset = 0; @@ -475,10 +417,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) switch (format_sel) { case NOTMUCH_FORMAT_TEXT: - format = &format_text; + format = NULL; break; case NOTMUCH_FORMAT_JSON: - format = &format_json; + format = sprinter_json_new (ctx, stdout); break; } @@ -546,5 +488,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) notmuch_query_destroy (query); notmuch_database_destroy (notmuch); + if (format != sprinter_text) + talloc_free(format); + return ret; } diff --git a/test/json b/test/json index 6439788..88b8a6d 100755 --- a/test/json +++ b/test/json @@ -10,14 +10,8 @@ test_expect_equal "$output" "[[[{\"id\": \"${gen_msg_id}\", \"match\": true, \"e test_begin_subtest "Search message: json" add_message "[subject]=\"json-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"json-search-message\"" output=$(notmuch search --format=json "json-search-message" | notmuch_search_sanitize) -test_expect_equal "$output" "[{\"thread\": \"XXX\", -\"timestamp\": 946728000, -\"date_relative\": \"2000-01-01\", -\"matched\": 1, -\"total\": 1, -\"authors\": \"Notmuch Test Suite\", -\"subject\": \"json-search-subject\", -\"tags\": [\"inbox\", \"unread\"]}]" +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-subject\", \"tags\": [\"inbox\", \"unread\"]} +]" test_begin_subtest "Show message: json, utf-8" add_message "[subject]=\"json-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\"" @@ -40,13 +34,7 @@ test_expect_equal "$output" "[[[{\"id\": \"$id\", \"match\": true, \"excluded\": test_begin_subtest "Search message: json, utf-8" add_message "[subject]=\"json-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\"" output=$(notmuch search --format=json "jsön-search-méssage" | notmuch_search_sanitize) -test_expect_equal "$output" "[{\"thread\": \"XXX\", -\"timestamp\": 946728000, -\"date_relative\": \"2000-01-01\", -\"matched\": 1, -\"total\": 1, -\"authors\": \"Notmuch Test Suite\", -\"subject\": \"json-search-utf8-body-sübjéct\", -\"tags\": [\"inbox\", \"unread\"]}]" +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-utf8-body-sübjéct\", \"tags\": [\"inbox\", \"unread\"]} +]" test_done diff --git a/test/search-output b/test/search-output index 8b57a43..f2650f7 100755 --- a/test/search-output +++ b/test/search-output @@ -37,30 +37,31 @@ test_expect_equal_file OUTPUT EXPECTED test_begin_subtest "--output=threads --format=json" notmuch search --format=json --output=threads '*' | sed -e s/\".*\"/\"THREADID\"/ >OUTPUT cat <<EOF >EXPECTED -["THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID", -"THREADID"] +["THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +, "THREADID" +] EOF test_expect_equal_file OUTPUT EXPECTED @@ -125,58 +126,59 @@ test_expect_equal_file OUTPUT EXPECTED test_begin_subtest "--output=messages --format=json" notmuch search --format=json --output=messages '*' >OUTPUT cat <<EOF >EXPECTED -["4EFC743A.3060609@april.org", -"877h1wv7mg.fsf@inf-8657.int-evry.fr", -"1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk", -"877htoqdbo.fsf@yoom.home.cworth.org", -"878we4qdqf.fsf@yoom.home.cworth.org", -"87aaykqe24.fsf@yoom.home.cworth.org", -"87bpj0qeng.fsf@yoom.home.cworth.org", -"87fx8cqf8v.fsf@yoom.home.cworth.org", -"87hbssqfix.fsf@yoom.home.cworth.org", -"87iqd8qgiz.fsf@yoom.home.cworth.org", -"87k4xoqgnl.fsf@yoom.home.cworth.org", -"87ocn0qh6d.fsf@yoom.home.cworth.org", -"87pr7gqidx.fsf@yoom.home.cworth.org", -"867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", -"1258532999-9316-1-git-send-email-keithp@keithp.com", -"86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", -"86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", -"ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com", -"86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", -"736613.51770.qm@web113505.mail.gq1.yahoo.com", -"1258520223-15328-1-git-send-email-jan@ryngle.com", -"ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com", -"1258510940-7018-1-git-send-email-stewart@flamingspork.com", -"yunzl6kd1w0.fsf@aiko.keithp.com", -"yun1vjwegii.fsf@aiko.keithp.com", -"yun3a4cegoa.fsf@aiko.keithp.com", -"1258509400-32511-1-git-send-email-stewart@flamingspork.com", -"1258506353-20352-1-git-send-email-stewart@flamingspork.com", -"20091118010116.GC25380@dottiness.seas.harvard.edu", -"20091118005829.GB25380@dottiness.seas.harvard.edu", -"20091118005040.GA25380@dottiness.seas.harvard.edu", -"cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com", -"1258500222-32066-1-git-send-email-ingmar@exherbo.org", -"20091117232137.GA7669@griffis1.net", -"20091118002059.067214ed@hikari", -"1258498485-sup-142@elly", -"f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com", -"f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com", -"1258496327-12086-1-git-send-email-jan@ryngle.com", -"1258493565-13508-1-git-send-email-keithp@keithp.com", -"yunaayketfm.fsf@aiko.keithp.com", -"yunbpj0etua.fsf@aiko.keithp.com", -"1258491078-29658-1-git-send-email-dottedmag@dottedmag.net", -"87fx8can9z.fsf@vertex.dottedmag", -"20091117203301.GV3165@dottiness.seas.harvard.edu", -"87lji4lx9v.fsf@yoom.home.cworth.org", -"cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com", -"87iqd9rn3l.fsf@vertex.dottedmag", -"20091117190054.GU3165@dottiness.seas.harvard.edu", -"87lji5cbwo.fsf@yoom.home.cworth.org", -"1258471718-6781-2-git-send-email-dottedmag@dottedmag.net", -"1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"] +["4EFC743A.3060609@april.org" +, "877h1wv7mg.fsf@inf-8657.int-evry.fr" +, "1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk" +, "877htoqdbo.fsf@yoom.home.cworth.org" +, "878we4qdqf.fsf@yoom.home.cworth.org" +, "87aaykqe24.fsf@yoom.home.cworth.org" +, "87bpj0qeng.fsf@yoom.home.cworth.org" +, "87fx8cqf8v.fsf@yoom.home.cworth.org" +, "87hbssqfix.fsf@yoom.home.cworth.org" +, "87iqd8qgiz.fsf@yoom.home.cworth.org" +, "87k4xoqgnl.fsf@yoom.home.cworth.org" +, "87ocn0qh6d.fsf@yoom.home.cworth.org" +, "87pr7gqidx.fsf@yoom.home.cworth.org" +, "867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" +, "1258532999-9316-1-git-send-email-keithp@keithp.com" +, "86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" +, "86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" +, "ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com" +, "86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" +, "736613.51770.qm@web113505.mail.gq1.yahoo.com" +, "1258520223-15328-1-git-send-email-jan@ryngle.com" +, "ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com" +, "1258510940-7018-1-git-send-email-stewart@flamingspork.com" +, "yunzl6kd1w0.fsf@aiko.keithp.com" +, "yun1vjwegii.fsf@aiko.keithp.com" +, "yun3a4cegoa.fsf@aiko.keithp.com" +, "1258509400-32511-1-git-send-email-stewart@flamingspork.com" +, "1258506353-20352-1-git-send-email-stewart@flamingspork.com" +, "20091118010116.GC25380@dottiness.seas.harvard.edu" +, "20091118005829.GB25380@dottiness.seas.harvard.edu" +, "20091118005040.GA25380@dottiness.seas.harvard.edu" +, "cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com" +, "1258500222-32066-1-git-send-email-ingmar@exherbo.org" +, "20091117232137.GA7669@griffis1.net" +, "20091118002059.067214ed@hikari" +, "1258498485-sup-142@elly" +, "f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com" +, "f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com" +, "1258496327-12086-1-git-send-email-jan@ryngle.com" +, "1258493565-13508-1-git-send-email-keithp@keithp.com" +, "yunaayketfm.fsf@aiko.keithp.com" +, "yunbpj0etua.fsf@aiko.keithp.com" +, "1258491078-29658-1-git-send-email-dottedmag@dottedmag.net" +, "87fx8can9z.fsf@vertex.dottedmag" +, "20091117203301.GV3165@dottiness.seas.harvard.edu" +, "87lji4lx9v.fsf@yoom.home.cworth.org" +, "cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com" +, "87iqd9rn3l.fsf@vertex.dottedmag" +, "20091117190054.GU3165@dottiness.seas.harvard.edu" +, "87lji5cbwo.fsf@yoom.home.cworth.org" +, "1258471718-6781-2-git-send-email-dottedmag@dottedmag.net" +, "1258471718-6781-1-git-send-email-dottedmag@dottedmag.net" +] EOF test_expect_equal_file OUTPUT EXPECTED @@ -242,59 +244,60 @@ test_expect_equal_file OUTPUT EXPECTED test_begin_subtest "--output=files --format=json" notmuch search --format=json --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT cat <<EOF >EXPECTED -["MAIL_DIR/cur/52:2,", -"MAIL_DIR/cur/53:2,", -"MAIL_DIR/cur/50:2,", -"MAIL_DIR/cur/49:2,", -"MAIL_DIR/cur/48:2,", -"MAIL_DIR/cur/47:2,", -"MAIL_DIR/cur/46:2,", -"MAIL_DIR/cur/45:2,", -"MAIL_DIR/cur/44:2,", -"MAIL_DIR/cur/43:2,", -"MAIL_DIR/cur/42:2,", -"MAIL_DIR/cur/41:2,", -"MAIL_DIR/cur/40:2,", -"MAIL_DIR/cur/39:2,", -"MAIL_DIR/cur/38:2,", -"MAIL_DIR/cur/37:2,", -"MAIL_DIR/cur/36:2,", -"MAIL_DIR/cur/35:2,", -"MAIL_DIR/cur/34:2,", -"MAIL_DIR/cur/33:2,", -"MAIL_DIR/cur/32:2,", -"MAIL_DIR/cur/31:2,", -"MAIL_DIR/cur/30:2,", -"MAIL_DIR/cur/29:2,", -"MAIL_DIR/cur/28:2,", -"MAIL_DIR/cur/27:2,", -"MAIL_DIR/cur/26:2,", -"MAIL_DIR/cur/25:2,", -"MAIL_DIR/cur/24:2,", -"MAIL_DIR/cur/23:2,", -"MAIL_DIR/cur/22:2,", -"MAIL_DIR/cur/21:2,", -"MAIL_DIR/cur/19:2,", -"MAIL_DIR/cur/18:2,", -"MAIL_DIR/cur/51:2,", -"MAIL_DIR/cur/20:2,", -"MAIL_DIR/cur/17:2,", -"MAIL_DIR/cur/16:2,", -"MAIL_DIR/cur/15:2,", -"MAIL_DIR/cur/14:2,", -"MAIL_DIR/cur/13:2,", -"MAIL_DIR/cur/12:2,", -"MAIL_DIR/cur/11:2,", -"MAIL_DIR/cur/10:2,", -"MAIL_DIR/cur/09:2,", -"MAIL_DIR/cur/08:2,", -"MAIL_DIR/cur/06:2,", -"MAIL_DIR/cur/05:2,", -"MAIL_DIR/cur/04:2,", -"MAIL_DIR/cur/03:2,", -"MAIL_DIR/cur/07:2,", -"MAIL_DIR/cur/02:2,", -"MAIL_DIR/cur/01:2,"] +["MAIL_DIR/cur/52:2," +, "MAIL_DIR/cur/53:2," +, "MAIL_DIR/cur/50:2," +, "MAIL_DIR/cur/49:2," +, "MAIL_DIR/cur/48:2," +, "MAIL_DIR/cur/47:2," +, "MAIL_DIR/cur/46:2," +, "MAIL_DIR/cur/45:2," +, "MAIL_DIR/cur/44:2," +, "MAIL_DIR/cur/43:2," +, "MAIL_DIR/cur/42:2," +, "MAIL_DIR/cur/41:2," +, "MAIL_DIR/cur/40:2," +, "MAIL_DIR/cur/39:2," +, "MAIL_DIR/cur/38:2," +, "MAIL_DIR/cur/37:2," +, "MAIL_DIR/cur/36:2," +, "MAIL_DIR/cur/35:2," +, "MAIL_DIR/cur/34:2," +, "MAIL_DIR/cur/33:2," +, "MAIL_DIR/cur/32:2," +, "MAIL_DIR/cur/31:2," +, "MAIL_DIR/cur/30:2," +, "MAIL_DIR/cur/29:2," +, "MAIL_DIR/cur/28:2," +, "MAIL_DIR/cur/27:2," +, "MAIL_DIR/cur/26:2," +, "MAIL_DIR/cur/25:2," +, "MAIL_DIR/cur/24:2," +, "MAIL_DIR/cur/23:2," +, "MAIL_DIR/cur/22:2," +, "MAIL_DIR/cur/21:2," +, "MAIL_DIR/cur/19:2," +, "MAIL_DIR/cur/18:2," +, "MAIL_DIR/cur/51:2," +, "MAIL_DIR/cur/20:2," +, "MAIL_DIR/cur/17:2," +, "MAIL_DIR/cur/16:2," +, "MAIL_DIR/cur/15:2," +, "MAIL_DIR/cur/14:2," +, "MAIL_DIR/cur/13:2," +, "MAIL_DIR/cur/12:2," +, "MAIL_DIR/cur/11:2," +, "MAIL_DIR/cur/10:2," +, "MAIL_DIR/cur/09:2," +, "MAIL_DIR/cur/08:2," +, "MAIL_DIR/cur/06:2," +, "MAIL_DIR/cur/05:2," +, "MAIL_DIR/cur/04:2," +, "MAIL_DIR/cur/03:2," +, "MAIL_DIR/cur/07:2," +, "MAIL_DIR/cur/02:2," +, "MAIL_DIR/cur/01:2," +] EOF test_expect_equal_file OUTPUT EXPECTED @@ -311,10 +314,11 @@ test_expect_equal_file OUTPUT EXPECTED test_begin_subtest "--output=tags --format=json" notmuch search --format=json --output=tags '*' >OUTPUT cat <<EOF >EXPECTED -["attachment", -"inbox", -"signed", -"unread"] +["attachment" +, "inbox" +, "signed" +, "unread" +] EOF test_expect_equal_file OUTPUT EXPECTED -- 1.7.11.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search. 2012-07-12 7:43 ` [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search craven @ 2012-07-13 0:02 ` Austin Clements 2012-07-13 0:10 ` Austin Clements 0 siblings, 1 reply; 18+ messages in thread From: Austin Clements @ 2012-07-13 0:02 UTC (permalink / raw) To: craven; +Cc: notmuch This is fantastic. It simplifies the code a lot, and I think it opens up opportunities to simplify it even further. Detailed comments are below, but first one general comment. For the text format, I wonder if most of the special case code would go away with a stub sprinter that did nothing for most operations and for string printed the string followed by a newline (or maybe the newline would be printed by the "frame" method or whatever you end up calling it). I believe this would unify all of the code in do_search_tags and do_search_files between the two formats. For do_search_threads, you'll still need some special-casing to format the summary line, but I think it would unify all of the framing code. (If this does work out, some of my comments below will be irrelevant.) Quoth craven@gmx.net on Jul 12 at 9:43 am: > This patch switches from the current ad-hoc printer to the structured > output formatter in sprinter.h. > > It removes search_format_t, replaces it by sprinter_t and inlines the > text printer where necessary. > > The tests are changed (only whitespaces and regular expressions) in > order to make them PASS for the new structured output formatter. > --- > notmuch-search.c | 349 ++++++++++++++++++++++------------------------------- > test/json | 20 +-- > test/search-output | 270 +++++++++++++++++++++-------------------- > 3 files changed, 288 insertions(+), 351 deletions(-) > > diff --git a/notmuch-search.c b/notmuch-search.c > index 3be296d..b853f5f 100644 > --- a/notmuch-search.c > +++ b/notmuch-search.c > @@ -19,6 +19,7 @@ > */ > > #include "notmuch-client.h" > +#include "sprinter.h" > > typedef enum { > OUTPUT_SUMMARY, > @@ -28,91 +29,9 @@ typedef enum { > OUTPUT_TAGS > } output_t; > > -typedef struct search_format { > - const char *results_start; > - const char *item_start; > - void (*item_id) (const void *ctx, > - const char *item_type, > - const char *item_id); > - void (*thread_summary) (const void *ctx, > - const char *thread_id, > - const time_t date, > - const int matched, > - const int total, > - const char *authors, > - const char *subject); > - const char *tag_start; > - const char *tag; > - const char *tag_sep; > - const char *tag_end; > - const char *item_sep; > - const char *item_end; > - const char *results_end; > - const char *results_null; > -} search_format_t; > - > -static void > -format_item_id_text (const void *ctx, > - const char *item_type, > - const char *item_id); > - > -static void > -format_thread_text (const void *ctx, > - const char *thread_id, > - const time_t date, > - const int matched, > - const int total, > - const char *authors, > - const char *subject); > -static const search_format_t format_text = { > - "", > - "", > - format_item_id_text, > - format_thread_text, > - " (", > - "%s", " ", > - ")", "\n", > - "", > - "\n", > - "", > -}; > - > -static void > -format_item_id_json (const void *ctx, > - const char *item_type, > - const char *item_id); > - > -static void > -format_thread_json (const void *ctx, > - const char *thread_id, > - const time_t date, > - const int matched, > - const int total, > - const char *authors, > - const char *subject); > - > -/* Any changes to the JSON format should be reflected in the file > - * devel/schemata. */ > -static const search_format_t format_json = { > - "[", > - "{", > - format_item_id_json, > - format_thread_json, > - "\"tags\": [", > - "\"%s\"", ", ", > - "]", ",\n", > - "}", > - "]\n", > - "]\n", > -}; > - > -static void > -format_item_id_text (unused (const void *ctx), > - const char *item_type, > - const char *item_id) > -{ > - printf ("%s%s", item_type, item_id); > -} > +static const char * text_item_sep = "\n"; > +static const char * text_results_null = ""; > +static const char * text_results_end = "\n"; Given that you're special-casing the text format anyway, I think it's actually more readable if you hard-code these in the appropriate places below. > > static char * > sanitize_string (const void *ctx, const char *str) > @@ -131,72 +50,8 @@ sanitize_string (const void *ctx, const char *str) > return out; > } > > -static void > -format_thread_text (const void *ctx, > - const char *thread_id, > - const time_t date, > - const int matched, > - const int total, > - const char *authors, > - const char *subject) > -{ > - void *ctx_quote = talloc_new (ctx); > - > - printf ("thread:%s %12s [%d/%d] %s; %s", > - thread_id, > - notmuch_time_relative_date (ctx, date), > - matched, > - total, > - sanitize_string (ctx_quote, authors), > - sanitize_string (ctx_quote, subject)); > - > - talloc_free (ctx_quote); > -} > - > -static void > -format_item_id_json (const void *ctx, > - unused (const char *item_type), > - const char *item_id) > -{ > - void *ctx_quote = talloc_new (ctx); > - > - printf ("%s", json_quote_str (ctx_quote, item_id)); > - > - talloc_free (ctx_quote); > - > -} > - > -static void > -format_thread_json (const void *ctx, > - const char *thread_id, > - const time_t date, > - const int matched, > - const int total, > - const char *authors, > - const char *subject) > -{ > - void *ctx_quote = talloc_new (ctx); > - > - printf ("\"thread\": %s,\n" > - "\"timestamp\": %ld,\n" > - "\"date_relative\": \"%s\",\n" > - "\"matched\": %d,\n" > - "\"total\": %d,\n" > - "\"authors\": %s,\n" > - "\"subject\": %s,\n", > - json_quote_str (ctx_quote, thread_id), > - date, > - notmuch_time_relative_date (ctx, date), > - matched, > - total, > - json_quote_str (ctx_quote, authors), > - json_quote_str (ctx_quote, subject)); > - > - talloc_free (ctx_quote); > -} > - > static int > -do_search_threads (const search_format_t *format, > +do_search_threads (sprinter_t *format, > notmuch_query_t *query, > notmuch_sort_t sort, > output_t output, > @@ -220,7 +75,9 @@ do_search_threads (const search_format_t *format, > if (threads == NULL) > return 1; > > - fputs (format->results_start, stdout); > + if (format != sprinter_text) { > + format->begin_list (format); > + } > > for (i = 0; > notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit); > @@ -235,43 +92,96 @@ do_search_threads (const search_format_t *format, > continue; > } > > - if (! first_thread) > - fputs (format->item_sep, stdout); > + if (format == sprinter_text && ! first_thread) > + fputs (text_item_sep, stdout); I think you can drop this (and the first_thread variable) if you print ")\n" instead of ")" down below. This roundabout first_thread stuff was necessary to support the JSON format because it uses separators between results; however, the text format is much simpler because it just has the terminating newline after each result. Hence, you can just bake printing the newline in with printing the result and you never have to worry about whether it's the first result of if you didn't print any results. > > if (output == OUTPUT_THREADS) { > - format->item_id (thread, "thread:", > - notmuch_thread_get_thread_id (thread)); > + const char *thread_id = notmuch_thread_get_thread_id (thread); > + if (format == sprinter_text) > + printf ("thread:%s", thread_id); > + else { > + format->string (format, thread_id); > + format->frame (format); > + } > + > } else { /* output == OUTPUT_SUMMARY */ > - fputs (format->item_start, stdout); > + const char *authors = notmuch_thread_get_authors (thread); > + const char *subject = notmuch_thread_get_subject (thread); > + const char *thread_id = notmuch_thread_get_thread_id (thread); > + int matched = notmuch_thread_get_matched_messages (thread); > + int total = notmuch_thread_get_total_messages (thread); > + const char *relative_date = NULL; > + > + if (format != sprinter_text) > + format->begin_map (format); > + Extra blank line. > > if (sort == NOTMUCH_SORT_OLDEST_FIRST) > date = notmuch_thread_get_oldest_date (thread); > else > date = notmuch_thread_get_newest_date (thread); > > - format->thread_summary (thread, > - notmuch_thread_get_thread_id (thread), > - date, > - notmuch_thread_get_matched_messages (thread), > - notmuch_thread_get_total_messages (thread), > - notmuch_thread_get_authors (thread), > - notmuch_thread_get_subject (thread)); > + void *ctx_quote = talloc_new (thread); > + relative_date = > + notmuch_time_relative_date (ctx_quote, date); > + > + if (format == sprinter_text) { > + printf ("thread:%s %12s [%d/%d] %s; %s", > + thread_id, > + relative_date, > + matched, > + total, > + sanitize_string (ctx_quote, authors), > + sanitize_string (ctx_quote, subject)); > + } else { > + format->map_key (format, "thread"); > + format->string (format, thread_id); > + format->map_key (format, "timestamp"); > + format->integer (format, date); > + format->map_key (format, "date_relative"); > + format->string (format, relative_date); > + format->map_key (format, "matched"); > + format->integer (format, matched); > + format->map_key (format, "total"); > + format->integer (format, total); > + format->map_key (format, "authors"); > + format->string (format, authors); > + format->map_key (format, "subject"); > + format->string (format, subject); > + } > + > + talloc_free (ctx_quote); > > - fputs (format->tag_start, stdout); > + if (format == sprinter_text) { > + fputs (" (", stdout); > + } else { > + format->map_key (format, "tags"); > + format->begin_list (format); > + } > > for (tags = notmuch_thread_get_tags (thread); > notmuch_tags_valid (tags); > notmuch_tags_move_to_next (tags)) > { > - if (! first_tag) > - fputs (format->tag_sep, stdout); > - printf (format->tag, notmuch_tags_get (tags)); > + const char *tag = notmuch_tags_get (tags); > + if (format == sprinter_text) { > + if (! first_tag) > + fputs (" ", stdout); > + fputs (tag, stdout); > + } else { > + format->string (format, tag); > + } > + > first_tag = 0; > } > > - fputs (format->tag_end, stdout); > - > - fputs (format->item_end, stdout); > + if (format == sprinter_text) { > + fputs (")", stdout); > + } else { > + format->end (format); > + format->end (format); > + format->frame (format); > + } > } > > first_thread = 0; > @@ -279,16 +189,20 @@ do_search_threads (const search_format_t *format, > notmuch_thread_destroy (thread); > } > > - if (first_thread) > - fputs (format->results_null, stdout); > - else > - fputs (format->results_end, stdout); > + if (format == sprinter_text) > + if (first_thread) > + fputs (text_results_null, stdout); > + else > + fputs (text_results_end, stdout); You can drop this, too, if you print ")\n" instead of ")" above. > + else { > + format->end (format); > + } > > return 0; > } > > static int > -do_search_messages (const search_format_t *format, > +do_search_messages (sprinter_t *format, > notmuch_query_t *query, > output_t output, > int offset, > @@ -310,7 +224,8 @@ do_search_messages (const search_format_t *format, > if (messages == NULL) > return 1; > > - fputs (format->results_start, stdout); > + if (format != sprinter_text) > + format->begin_list (format); > > for (i = 0; > notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit); > @@ -328,23 +243,36 @@ do_search_messages (const search_format_t *format, > notmuch_filenames_valid (filenames); > notmuch_filenames_move_to_next (filenames)) > { > - if (! first_message) > - fputs (format->item_sep, stdout); > + const char *filenames_str = notmuch_filenames_get (filenames); > + > + if (format == sprinter_text && ! first_message) > + fputs (text_item_sep, stdout); Likewise, you can remove this, the first_message logic for OUTPUT_MESSAGES, and the first_message logic at the end of this function if you print a newline after filenames_str and message_id. > > - format->item_id (message, "", > - notmuch_filenames_get (filenames)); > + if (format == sprinter_text) > + fputs (filenames_str, stdout); > + else { > + format->string (format, filenames_str); > + format->frame (format); > + } > > first_message = 0; > } > - > - notmuch_filenames_destroy( filenames ); > + > + notmuch_filenames_destroy (filenames); No need to reformat this. > > } else { /* output == OUTPUT_MESSAGES */ > - if (! first_message) > - fputs (format->item_sep, stdout); > + const char *message_id = notmuch_message_get_message_id (message); > + > + if (format == sprinter_text && ! first_message) > + fputs (text_item_sep, stdout); > + > + if (format == sprinter_text) > + printf ("id:%s", message_id); > + else { > + format->string (format, message_id); > + format->frame (format); > + } > > - format->item_id (message, "id:", > - notmuch_message_get_message_id (message)); > first_message = 0; > } > > @@ -353,17 +281,21 @@ do_search_messages (const search_format_t *format, > > notmuch_messages_destroy (messages); > > - if (first_message) > - fputs (format->results_null, stdout); > - else > - fputs (format->results_end, stdout); > + if (format == sprinter_text) > + if (first_message) > + fputs (text_results_null, stdout); > + else > + fputs (text_results_end, stdout); > + else { > + format->end (format); > + } > > return 0; > } > > static int > do_search_tags (notmuch_database_t *notmuch, > - const search_format_t *format, > + sprinter_t *format, > notmuch_query_t *query) > { > notmuch_messages_t *messages = NULL; > @@ -387,7 +319,8 @@ do_search_tags (notmuch_database_t *notmuch, > if (tags == NULL) > return 1; > > - fputs (format->results_start, stdout); > + if (format != sprinter_text) > + format->begin_list (format); > > for (; > notmuch_tags_valid (tags); > @@ -395,10 +328,15 @@ do_search_tags (notmuch_database_t *notmuch, > { > tag = notmuch_tags_get (tags); > > - if (! first_tag) > - fputs (format->item_sep, stdout); > + if (format == sprinter_text && ! first_tag) > + fputs (text_item_sep, stdout); Same thing about dropping the first_tag logic in this function if you print a newline after tag below. > > - format->item_id (tags, "", tag); > + if (format == sprinter_text) > + fputs (tag, stdout); > + else { > + format->string (format, tag); > + format->frame (format); > + } > > first_tag = 0; > } > @@ -408,10 +346,14 @@ do_search_tags (notmuch_database_t *notmuch, > if (messages) > notmuch_messages_destroy (messages); > > - if (first_tag) > - fputs (format->results_null, stdout); > - else > - fputs (format->results_end, stdout); > + if (format == sprinter_text) > + if (first_tag) > + fputs (text_results_null, stdout); > + else > + fputs (text_results_end, stdout); > + else { > + format->end (format); > + } > > return 0; > } > @@ -430,7 +372,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) > notmuch_query_t *query; > char *query_str; > notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST; > - const search_format_t *format = &format_text; > + sprinter_t *format = NULL; /* the default output is text */ > int opt_index, ret; > output_t output = OUTPUT_SUMMARY; > int offset = 0; > @@ -475,10 +417,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) > > switch (format_sel) { > case NOTMUCH_FORMAT_TEXT: > - format = &format_text; > + format = NULL; > break; > case NOTMUCH_FORMAT_JSON: > - format = &format_json; > + format = sprinter_json_new (ctx, stdout); > break; > } > > @@ -546,5 +488,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) > notmuch_query_destroy (query); > notmuch_database_destroy (notmuch); > > + if (format != sprinter_text) > + talloc_free(format); Missing space before argument list. > + > return ret; > } > diff --git a/test/json b/test/json > index 6439788..88b8a6d 100755 > --- a/test/json > +++ b/test/json > @@ -10,14 +10,8 @@ test_expect_equal "$output" "[[[{\"id\": \"${gen_msg_id}\", \"match\": true, \"e > test_begin_subtest "Search message: json" > add_message "[subject]=\"json-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"json-search-message\"" > output=$(notmuch search --format=json "json-search-message" | notmuch_search_sanitize) If you additionally pipe this through notmuch_json_show_sanitize (or maybe define a similar function for search), then the output will be much closer to the output currently in the test. > -test_expect_equal "$output" "[{\"thread\": \"XXX\", > -\"timestamp\": 946728000, > -\"date_relative\": \"2000-01-01\", > -\"matched\": 1, > -\"total\": 1, > -\"authors\": \"Notmuch Test Suite\", > -\"subject\": \"json-search-subject\", > -\"tags\": [\"inbox\", \"unread\"]}]" > +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-subject\", \"tags\": [\"inbox\", \"unread\"]} > +]" > > test_begin_subtest "Show message: json, utf-8" > add_message "[subject]=\"json-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\"" > @@ -40,13 +34,7 @@ test_expect_equal "$output" "[[[{\"id\": \"$id\", \"match\": true, \"excluded\": > test_begin_subtest "Search message: json, utf-8" > add_message "[subject]=\"json-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\"" > output=$(notmuch search --format=json "jsön-search-méssage" | notmuch_search_sanitize) Same here. > -test_expect_equal "$output" "[{\"thread\": \"XXX\", > -\"timestamp\": 946728000, > -\"date_relative\": \"2000-01-01\", > -\"matched\": 1, > -\"total\": 1, > -\"authors\": \"Notmuch Test Suite\", > -\"subject\": \"json-search-utf8-body-sübjéct\", > -\"tags\": [\"inbox\", \"unread\"]}]" > +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-utf8-body-sübjéct\", \"tags\": [\"inbox\", \"unread\"]} > +]" > > test_done > diff --git a/test/search-output b/test/search-output > index 8b57a43..f2650f7 100755 > --- a/test/search-output > +++ b/test/search-output > @@ -37,30 +37,31 @@ test_expect_equal_file OUTPUT EXPECTED > test_begin_subtest "--output=threads --format=json" > notmuch search --format=json --output=threads '*' | sed -e s/\".*\"/\"THREADID\"/ >OUTPUT > cat <<EOF >EXPECTED > -["THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID", > -"THREADID"] > +["THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +, "THREADID" > +] Hmm. It would be nice if json_frame printed this in the way people usually format JSON and the test currently expects. Doing this isn't entirely straightforward because the caller could call frame and then immediately call end, so you can't just print ",\n"... You could replace json_state.first with /* The separator to print before the next value. */ const char *sep; json_begin_value would fputs sep and set it to ", ", json_begin_aggregate would set it to "", and json_map_key would set it to ": " and not fputs(": ", spj->stream) itself. json_frame could then be something like static void json_frame(struct sprinter *sp) { struct json_state *state = ((struct sprinter_json*)sp)->state; if (!state) return; if (state->sep[0] == ',') state->sep = ",\n"; else if (state->sep[0] == ':') state->sep = ":\n"; } I think that would work. > EOF > test_expect_equal_file OUTPUT EXPECTED > > @@ -125,58 +126,59 @@ test_expect_equal_file OUTPUT EXPECTED > test_begin_subtest "--output=messages --format=json" > notmuch search --format=json --output=messages '*' >OUTPUT > cat <<EOF >EXPECTED > -["4EFC743A.3060609@april.org", > -"877h1wv7mg.fsf@inf-8657.int-evry.fr", > -"1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk", > -"877htoqdbo.fsf@yoom.home.cworth.org", > -"878we4qdqf.fsf@yoom.home.cworth.org", > -"87aaykqe24.fsf@yoom.home.cworth.org", > -"87bpj0qeng.fsf@yoom.home.cworth.org", > -"87fx8cqf8v.fsf@yoom.home.cworth.org", > -"87hbssqfix.fsf@yoom.home.cworth.org", > -"87iqd8qgiz.fsf@yoom.home.cworth.org", > -"87k4xoqgnl.fsf@yoom.home.cworth.org", > -"87ocn0qh6d.fsf@yoom.home.cworth.org", > -"87pr7gqidx.fsf@yoom.home.cworth.org", > -"867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", > -"1258532999-9316-1-git-send-email-keithp@keithp.com", > -"86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", > -"86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", > -"ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com", > -"86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me", > -"736613.51770.qm@web113505.mail.gq1.yahoo.com", > -"1258520223-15328-1-git-send-email-jan@ryngle.com", > -"ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com", > -"1258510940-7018-1-git-send-email-stewart@flamingspork.com", > -"yunzl6kd1w0.fsf@aiko.keithp.com", > -"yun1vjwegii.fsf@aiko.keithp.com", > -"yun3a4cegoa.fsf@aiko.keithp.com", > -"1258509400-32511-1-git-send-email-stewart@flamingspork.com", > -"1258506353-20352-1-git-send-email-stewart@flamingspork.com", > -"20091118010116.GC25380@dottiness.seas.harvard.edu", > -"20091118005829.GB25380@dottiness.seas.harvard.edu", > -"20091118005040.GA25380@dottiness.seas.harvard.edu", > -"cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com", > -"1258500222-32066-1-git-send-email-ingmar@exherbo.org", > -"20091117232137.GA7669@griffis1.net", > -"20091118002059.067214ed@hikari", > -"1258498485-sup-142@elly", > -"f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com", > -"f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com", > -"1258496327-12086-1-git-send-email-jan@ryngle.com", > -"1258493565-13508-1-git-send-email-keithp@keithp.com", > -"yunaayketfm.fsf@aiko.keithp.com", > -"yunbpj0etua.fsf@aiko.keithp.com", > -"1258491078-29658-1-git-send-email-dottedmag@dottedmag.net", > -"87fx8can9z.fsf@vertex.dottedmag", > -"20091117203301.GV3165@dottiness.seas.harvard.edu", > -"87lji4lx9v.fsf@yoom.home.cworth.org", > -"cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com", > -"87iqd9rn3l.fsf@vertex.dottedmag", > -"20091117190054.GU3165@dottiness.seas.harvard.edu", > -"87lji5cbwo.fsf@yoom.home.cworth.org", > -"1258471718-6781-2-git-send-email-dottedmag@dottedmag.net", > -"1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"] > +["4EFC743A.3060609@april.org" > +, "877h1wv7mg.fsf@inf-8657.int-evry.fr" > +, "1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk" > +, "877htoqdbo.fsf@yoom.home.cworth.org" > +, "878we4qdqf.fsf@yoom.home.cworth.org" > +, "87aaykqe24.fsf@yoom.home.cworth.org" > +, "87bpj0qeng.fsf@yoom.home.cworth.org" > +, "87fx8cqf8v.fsf@yoom.home.cworth.org" > +, "87hbssqfix.fsf@yoom.home.cworth.org" > +, "87iqd8qgiz.fsf@yoom.home.cworth.org" > +, "87k4xoqgnl.fsf@yoom.home.cworth.org" > +, "87ocn0qh6d.fsf@yoom.home.cworth.org" > +, "87pr7gqidx.fsf@yoom.home.cworth.org" > +, "867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" > +, "1258532999-9316-1-git-send-email-keithp@keithp.com" > +, "86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" > +, "86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" > +, "ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com" > +, "86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me" > +, "736613.51770.qm@web113505.mail.gq1.yahoo.com" > +, "1258520223-15328-1-git-send-email-jan@ryngle.com" > +, "ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com" > +, "1258510940-7018-1-git-send-email-stewart@flamingspork.com" > +, "yunzl6kd1w0.fsf@aiko.keithp.com" > +, "yun1vjwegii.fsf@aiko.keithp.com" > +, "yun3a4cegoa.fsf@aiko.keithp.com" > +, "1258509400-32511-1-git-send-email-stewart@flamingspork.com" > +, "1258506353-20352-1-git-send-email-stewart@flamingspork.com" > +, "20091118010116.GC25380@dottiness.seas.harvard.edu" > +, "20091118005829.GB25380@dottiness.seas.harvard.edu" > +, "20091118005040.GA25380@dottiness.seas.harvard.edu" > +, "cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com" > +, "1258500222-32066-1-git-send-email-ingmar@exherbo.org" > +, "20091117232137.GA7669@griffis1.net" > +, "20091118002059.067214ed@hikari" > +, "1258498485-sup-142@elly" > +, "f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com" > +, "f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com" > +, "1258496327-12086-1-git-send-email-jan@ryngle.com" > +, "1258493565-13508-1-git-send-email-keithp@keithp.com" > +, "yunaayketfm.fsf@aiko.keithp.com" > +, "yunbpj0etua.fsf@aiko.keithp.com" > +, "1258491078-29658-1-git-send-email-dottedmag@dottedmag.net" > +, "87fx8can9z.fsf@vertex.dottedmag" > +, "20091117203301.GV3165@dottiness.seas.harvard.edu" > +, "87lji4lx9v.fsf@yoom.home.cworth.org" > +, "cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com" > +, "87iqd9rn3l.fsf@vertex.dottedmag" > +, "20091117190054.GU3165@dottiness.seas.harvard.edu" > +, "87lji5cbwo.fsf@yoom.home.cworth.org" > +, "1258471718-6781-2-git-send-email-dottedmag@dottedmag.net" > +, "1258471718-6781-1-git-send-email-dottedmag@dottedmag.net" > +] > EOF > test_expect_equal_file OUTPUT EXPECTED > > @@ -242,59 +244,60 @@ test_expect_equal_file OUTPUT EXPECTED > test_begin_subtest "--output=files --format=json" > notmuch search --format=json --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT > cat <<EOF >EXPECTED > -["MAIL_DIR/cur/52:2,", > -"MAIL_DIR/cur/53:2,", > -"MAIL_DIR/cur/50:2,", > -"MAIL_DIR/cur/49:2,", > -"MAIL_DIR/cur/48:2,", > -"MAIL_DIR/cur/47:2,", > -"MAIL_DIR/cur/46:2,", > -"MAIL_DIR/cur/45:2,", > -"MAIL_DIR/cur/44:2,", > -"MAIL_DIR/cur/43:2,", > -"MAIL_DIR/cur/42:2,", > -"MAIL_DIR/cur/41:2,", > -"MAIL_DIR/cur/40:2,", > -"MAIL_DIR/cur/39:2,", > -"MAIL_DIR/cur/38:2,", > -"MAIL_DIR/cur/37:2,", > -"MAIL_DIR/cur/36:2,", > -"MAIL_DIR/cur/35:2,", > -"MAIL_DIR/cur/34:2,", > -"MAIL_DIR/cur/33:2,", > -"MAIL_DIR/cur/32:2,", > -"MAIL_DIR/cur/31:2,", > -"MAIL_DIR/cur/30:2,", > -"MAIL_DIR/cur/29:2,", > -"MAIL_DIR/cur/28:2,", > -"MAIL_DIR/cur/27:2,", > -"MAIL_DIR/cur/26:2,", > -"MAIL_DIR/cur/25:2,", > -"MAIL_DIR/cur/24:2,", > -"MAIL_DIR/cur/23:2,", > -"MAIL_DIR/cur/22:2,", > -"MAIL_DIR/cur/21:2,", > -"MAIL_DIR/cur/19:2,", > -"MAIL_DIR/cur/18:2,", > -"MAIL_DIR/cur/51:2,", > -"MAIL_DIR/cur/20:2,", > -"MAIL_DIR/cur/17:2,", > -"MAIL_DIR/cur/16:2,", > -"MAIL_DIR/cur/15:2,", > -"MAIL_DIR/cur/14:2,", > -"MAIL_DIR/cur/13:2,", > -"MAIL_DIR/cur/12:2,", > -"MAIL_DIR/cur/11:2,", > -"MAIL_DIR/cur/10:2,", > -"MAIL_DIR/cur/09:2,", > -"MAIL_DIR/cur/08:2,", > -"MAIL_DIR/cur/06:2,", > -"MAIL_DIR/cur/05:2,", > -"MAIL_DIR/cur/04:2,", > -"MAIL_DIR/cur/03:2,", > -"MAIL_DIR/cur/07:2,", > -"MAIL_DIR/cur/02:2,", > -"MAIL_DIR/cur/01:2,"] > +["MAIL_DIR/cur/52:2," > +, "MAIL_DIR/cur/53:2," > +, "MAIL_DIR/cur/50:2," > +, "MAIL_DIR/cur/49:2," > +, "MAIL_DIR/cur/48:2," > +, "MAIL_DIR/cur/47:2," > +, "MAIL_DIR/cur/46:2," > +, "MAIL_DIR/cur/45:2," > +, "MAIL_DIR/cur/44:2," > +, "MAIL_DIR/cur/43:2," > +, "MAIL_DIR/cur/42:2," > +, "MAIL_DIR/cur/41:2," > +, "MAIL_DIR/cur/40:2," > +, "MAIL_DIR/cur/39:2," > +, "MAIL_DIR/cur/38:2," > +, "MAIL_DIR/cur/37:2," > +, "MAIL_DIR/cur/36:2," > +, "MAIL_DIR/cur/35:2," > +, "MAIL_DIR/cur/34:2," > +, "MAIL_DIR/cur/33:2," > +, "MAIL_DIR/cur/32:2," > +, "MAIL_DIR/cur/31:2," > +, "MAIL_DIR/cur/30:2," > +, "MAIL_DIR/cur/29:2," > +, "MAIL_DIR/cur/28:2," > +, "MAIL_DIR/cur/27:2," > +, "MAIL_DIR/cur/26:2," > +, "MAIL_DIR/cur/25:2," > +, "MAIL_DIR/cur/24:2," > +, "MAIL_DIR/cur/23:2," > +, "MAIL_DIR/cur/22:2," > +, "MAIL_DIR/cur/21:2," > +, "MAIL_DIR/cur/19:2," > +, "MAIL_DIR/cur/18:2," > +, "MAIL_DIR/cur/51:2," > +, "MAIL_DIR/cur/20:2," > +, "MAIL_DIR/cur/17:2," > +, "MAIL_DIR/cur/16:2," > +, "MAIL_DIR/cur/15:2," > +, "MAIL_DIR/cur/14:2," > +, "MAIL_DIR/cur/13:2," > +, "MAIL_DIR/cur/12:2," > +, "MAIL_DIR/cur/11:2," > +, "MAIL_DIR/cur/10:2," > +, "MAIL_DIR/cur/09:2," > +, "MAIL_DIR/cur/08:2," > +, "MAIL_DIR/cur/06:2," > +, "MAIL_DIR/cur/05:2," > +, "MAIL_DIR/cur/04:2," > +, "MAIL_DIR/cur/03:2," > +, "MAIL_DIR/cur/07:2," > +, "MAIL_DIR/cur/02:2," > +, "MAIL_DIR/cur/01:2," > +] > EOF > test_expect_equal_file OUTPUT EXPECTED > > @@ -311,10 +314,11 @@ test_expect_equal_file OUTPUT EXPECTED > test_begin_subtest "--output=tags --format=json" > notmuch search --format=json --output=tags '*' >OUTPUT > cat <<EOF >EXPECTED > -["attachment", > -"inbox", > -"signed", > -"unread"] > +["attachment" > +, "inbox" > +, "signed" > +, "unread" > +] > EOF > test_expect_equal_file OUTPUT EXPECTED > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search. 2012-07-13 0:02 ` Austin Clements @ 2012-07-13 0:10 ` Austin Clements 0 siblings, 0 replies; 18+ messages in thread From: Austin Clements @ 2012-07-13 0:10 UTC (permalink / raw) To: craven; +Cc: notmuch Quoth myself on Jul 12 at 8:02 pm: > This is fantastic. It simplifies the code a lot, and I think it opens > up opportunities to simplify it even further. > > Detailed comments are below, but first one general comment. For the > text format, I wonder if most of the special case code would go away > with a stub sprinter that did nothing for most operations and for > string printed the string followed by a newline (or maybe the newline > would be printed by the "frame" method or whatever you end up calling > it). I believe this would unify all of the code in do_search_tags and > do_search_files between the two formats. For do_search_threads, > you'll still need some special-casing to format the summary line, but > I think it would unify all of the framing code. (If this does work > out, some of my comments below will be irrelevant.) Oh, wait, the text format adds prefixes... I think something along the lines of what I described above would still simplify things. I can think of two ways to do it. You could have a completely stub sprinter where everything is a no-op; that would at least save the predication of calls like format->begin_list (). Or, you could have an additional function for the text formatter that registers a prefix, and have the string method first print that prefix and then the argument string. This additional function doesn't have to be a method of sprinter; it could just be a regular function that stashes the prefix away somewhere (a global variable's probably fine; if you want to be fancy you store it in an sprinter_text extension of sprinter_t like the one JSON uses). For the JSON format, this would be a no-op, so you could call it regardless of what format you've selected. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-07-13 0:10 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-30 2:59 Proposed structure formatter API Austin Clements 2012-07-12 7:43 ` Structured Formatters for JSON Output craven 2012-07-12 7:43 ` [PATCH v4 1/3] Add support for structured output formatters craven 2012-07-12 9:14 ` Mark Walters 2012-07-12 9:21 ` Mark Walters 2012-07-12 11:22 ` craven 2012-07-12 11:25 ` Mark Walters 2012-07-12 12:05 ` Tomi Ollila 2012-07-12 10:05 ` Tomi Ollila 2012-07-12 20:29 ` Austin Clements 2012-07-12 22:08 ` Austin Clements 2012-07-12 7:43 ` [PATCH v4 2/3] Add structured output formatter for JSON craven 2012-07-12 10:10 ` Tomi Ollila 2012-07-12 20:34 ` Austin Clements 2012-07-12 22:07 ` Austin Clements 2012-07-12 7:43 ` [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search craven 2012-07-13 0:02 ` Austin Clements 2012-07-13 0:10 ` Austin Clements
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).