[PATCH] rewriting notmuch-search for structured output to make other output formats easier

[PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Peter Feigl]

The output routines have been rewritten so that logical structure
(objects with key/value pairs, arrays, strings and numbers) are
written instead of ad-hoc printfs. This allows for easier adaptation
of other output formats, as only the routines that start/end an object
etc. have to be rewritten. The logic is the same for all formats.
The default text output is handled differently, special cases are
inserted at the proper places, as it differs too much from the
structured output.
---
 notmuch-search.c |  493 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 309 insertions(+), 184 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 8867aab..bce44c2 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -29,88 +29,221 @@ typedef enum {
 } 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;
+    void (*start_object) (const void *ctx, FILE *stream);
+    void (*end_object) (const void *ctx, FILE *stream);
+    void (*start_attribute) (const void *ctx, FILE *stream);
+    void (*attribute_key) (const void *ctx, FILE *stream, const char *key);
+    void (*attribute_key_value_separator) (const void *ctx, FILE *stream);
+    void (*end_attribute) (const void *ctx, FILE *stream);
+    void (*attribute_separator) (const void *ctx, FILE *stream);
+    void (*start_array) (const void *ctx, FILE *stream);
+    void (*array_item_separator) (const void *ctx, FILE *stream);
+    void (*end_array) (const void *ctx, FILE *stream);
+    void (*number) (const void *ctx, FILE *stream, int number);
+    void (*string) (const void *ctx, FILE *stream, const char *string);
+    void (*boolean) (const void *ctx, FILE *stream, int boolean);
 } 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);
+/* dummy format */
 static const search_format_t format_text = {
-    "",
-	"",
-	    format_item_id_text,
-	    format_thread_text,
-	    " (",
-		"%s", " ",
-	    ")", "\n",
-	"",
-    "\n",
-    "",
-};
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+
+void json_start_object(const void *ctx, FILE *stream);
+void json_end_object(const void *ctx, FILE *stream);
+void json_start_attribute(const void *ctx, FILE *stream);
+void json_attribute_key(const void *ctx, FILE *stream, const char *key);
+void json_attribute_key_value_separator(const void *ctx, FILE *stream);
+void json_end_attribute(const void *ctx, FILE *stream);
+void json_attribute_separator(const void *ctx, FILE *stream);
+void json_start_array(const void *ctx, FILE *stream);
+void json_array_item_separator(const void *ctx, FILE *stream);
+void json_end_array(const void *ctx, FILE *stream);
+void json_number(const void *ctx, FILE *stream, int number);
+void json_string(const void *ctx, FILE *stream, const char *string);
+void json_boolean(const void *ctx, FILE *stream, int boolean);
+
+
+void json_start_object(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("{", stream);
+}
+void json_end_object(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("}\n", stream);
+}
+void json_start_attribute(const void *ctx, FILE *stream) {
+    (void)ctx;
+    (void)stream;
+}
+void json_attribute_key(const void *ctx, FILE *stream, const char *key) {
+    (void)ctx;
+    fprintf(stream, "\"%s\"", key);
+}
+void json_attribute_key_value_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(": ", stream);
+}
+void json_end_attribute(const void *ctx, FILE *stream) {
+    (void)ctx;
+    (void)stream;
+}
+void json_attribute_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(",\n", stream);
+}
+void json_start_array(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("[", stream);
+}
+void json_array_item_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(", ", stream);
+}
+void json_end_array(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("]", stream);
+}
+void json_number(const void *ctx, FILE *stream, int number) {
+    (void)ctx;
+    fprintf(stream, "%i", number);
+}
+void json_string(const void *ctx, FILE *stream, const char *string) {
+    void *ctx_quote = talloc_new (ctx);    
+    fprintf(stream, "%s", json_quote_str (ctx_quote, string));
+    talloc_free (ctx_quote);
+}
+void json_boolean(const void *ctx, FILE *stream, int boolean) {
+    (void)ctx;
+    if(boolean)
+	fputs("true", stream);
+    else
+	fputs("false", stream);
+}
+
+/* helper functions for attributes */
+void format_attribute_string(const void *ctx, FILE *stream, const search_format_t *format, const char *key, const char *value);
+void format_attribute_number(const void *ctx, FILE *stream, const search_format_t *format, const char *key, int value);
+void format_attribute_boolean(const void *ctx, FILE *stream, const search_format_t *format, const char *key, const char *value);
+
+void format_attribute_string(const void *ctx, FILE *stream, const search_format_t *format, const char *key, const char *value) {
+    format->start_attribute(ctx, stream);
+    format->attribute_key(ctx, stream, key);
+    format->attribute_key_value_separator(ctx, stream);
+    format->string(ctx, stream, value);
+    format->end_attribute(ctx, stream);
+}
+
+void format_attribute_number(const void *ctx, FILE *stream, const search_format_t *format, const char *key, int value) {
+    format->start_attribute(ctx, stream);
+    format->attribute_key(ctx, stream, key);
+    format->attribute_key_value_separator(ctx, stream);
+    format->number(ctx, stream, value);
+    format->end_attribute(ctx, stream);
+}
 
-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);
 static const search_format_t format_json = {
-    "[",
-	"{",
-	    format_item_id_json,
-	    format_thread_json,
-	    "\"tags\": [",
-		"\"%s\"", ", ",
-	    "]", ",\n",
-	"}",
-    "]\n",
-    "]\n",
+    json_start_object,
+    json_end_object,
+    json_start_attribute,
+    json_attribute_key,
+    json_attribute_key_value_separator,
+    json_end_attribute,
+    json_attribute_separator,
+    json_start_array,
+    json_array_item_separator,
+    json_end_array,
+    json_number,
+    json_string,
+    json_boolean,
 };
 
-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);
+void sexp_start_object(const void *ctx, FILE *stream);
+void sexp_end_object(const void *ctx, FILE *stream);
+void sexp_start_attribute(const void *ctx, FILE *stream);
+void sexp_attribute_key(const void *ctx, FILE *stream, const char *key);
+void sexp_attribute_key_value_separator(const void *ctx, FILE *stream);
+void sexp_end_attribute(const void *ctx, FILE *stream);
+void sexp_attribute_separator(const void *ctx, FILE *stream);
+void sexp_start_array(const void *ctx, FILE *stream);
+void sexp_array_item_separator(const void *ctx, FILE *stream);
+void sexp_end_array(const void *ctx, FILE *stream);
+void sexp_number(const void *ctx, FILE *stream, int number);
+void sexp_string(const void *ctx, FILE *stream, const char *string);
+void sexp_boolean(const void *ctx, FILE *stream, int boolean);
+
+void sexp_start_object(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("(", stream);
+}
+void sexp_end_object(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(")\n", stream);
+}
+void sexp_start_attribute(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("(", stream);
+}
+void sexp_attribute_key(const void *ctx, FILE *stream, const char *key) {
+    (void)ctx;
+    fprintf(stream, "%s", key);
+}
+void sexp_attribute_key_value_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(" ", stream);
+}
+void sexp_end_attribute(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(")\n", stream);
+}
+void sexp_attribute_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(" ", stream);
+}
+void sexp_start_array(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs("(", stream);
+}
+void sexp_array_item_separator(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(" ", stream);
+}
+void sexp_end_array(const void *ctx, FILE *stream) {
+    (void)ctx;
+    fputs(")", stream);
+}
+void sexp_number(const void *ctx, FILE *stream, int number) {
+    (void)ctx;
+    fprintf(stream, "%i", number);
+}
+void sexp_string(const void *ctx, FILE *stream, const char *string) {
+    void *ctx_quote = talloc_new (ctx);    
+    fprintf(stream, "%s", json_quote_str (ctx_quote, string));
+    talloc_free (ctx_quote);
+}
+void sexp_boolean(const void *ctx, FILE *stream, int boolean) {
+    (void)ctx;
+    if(boolean)
+	fputs("#t", stream);
+    else
+	fputs("#f", stream);
 }
 
+static const search_format_t format_sexp = {
+    sexp_start_object,
+    sexp_end_object,
+    sexp_start_attribute,
+    sexp_attribute_key,
+    sexp_attribute_key_value_separator,
+    sexp_end_attribute,
+    sexp_attribute_separator,
+    sexp_start_array,
+    sexp_array_item_separator,
+    sexp_end_array,
+    sexp_number,
+    sexp_string,
+    sexp_boolean,
+};
+
 static char *
 sanitize_string (const void *ctx, const char *str)
 {
@@ -128,70 +261,6 @@ 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,
 		   notmuch_query_t *query,
@@ -217,7 +286,8 @@ do_search_threads (const search_format_t *format,
     if (threads == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if(format != &format_text)
+	format->start_array(threads, stdout);
 
     for (i = 0;
 	 notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);
@@ -232,43 +302,82 @@ do_search_threads (const search_format_t *format,
 	    continue;
 	}
 
-	if (! first_thread)
-	    fputs (format->item_sep, stdout);
+	if (! first_thread && format != &format_text)
+	    format->array_item_separator(thread, 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 != &format_text)
+		//format_attribute_string(thread, stdout, format, "thread", thread_id);
+		format->string(thread, stdout, thread_id);
+	    else /* text format */
+		printf("thread:%s\n", notmuch_thread_get_thread_id (thread));
 	} else { /* output == OUTPUT_SUMMARY */
-	    fputs (format->item_start, stdout);
+	    if(format != &format_text)
+		format->start_object(thread, stdout);
 
 	    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));
-
-	    fputs (format->tag_start, stdout);
+	    if(format != &format_text) {
+		format_attribute_string(thread, stdout, format, "thread", notmuch_thread_get_thread_id (thread));
+		format->attribute_separator(thread, stdout);
+		format_attribute_number(thread, stdout, format, "timestamp", date);
+		format->attribute_separator(thread, stdout);
+/*		format_attribute_string(thread, stdout, format, "date_relative", notmuch_time_relative_date (thread, date)); */
+/*		format->attribute_separator(thread, stdout); */
+		format_attribute_number(thread, stdout, format, "matched", notmuch_thread_get_matched_messages (thread));
+		format->attribute_separator(thread, stdout);
+		format_attribute_number(thread, stdout, format, "total", notmuch_thread_get_total_messages (thread));
+		format->attribute_separator(thread, stdout);
+		format_attribute_string(thread, stdout, format, "authors", notmuch_thread_get_authors (thread));
+		format->attribute_separator(thread, stdout);
+		format_attribute_string(thread, stdout, format, "subject", notmuch_thread_get_subject (thread));
+		format->attribute_separator(thread, stdout);
+
+		format->start_attribute(thread, stdout);
+		format->attribute_key(thread, stdout, "tags");
+		format->attribute_key_value_separator(thread, stdout);
+		format->start_array(thread, stdout);
+	    } else { /* text format */
+		void *ctx_quote = talloc_new (thread);
+		printf ("thread:%s %12s [%d/%d] %s; %s (",
+			notmuch_thread_get_thread_id (thread),
+			notmuch_time_relative_date (ctx_quote, date),
+			notmuch_thread_get_matched_messages (thread),
+			notmuch_thread_get_total_messages (thread),
+			sanitize_string (ctx_quote, notmuch_thread_get_authors (thread)),
+			sanitize_string (ctx_quote, notmuch_thread_get_subject (thread)));
+		talloc_free (ctx_quote);
+	    }
 
 	    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));
+		if (! first_tag) {
+		    if(format != &format_text)
+			format->array_item_separator(thread, stdout);
+		    else /* text format */
+			printf(" ");
+		}
+		if(format != &format_text)
+		    format->string(thread, stdout, notmuch_tags_get(tags));
+		else /* text format */
+		    printf("%s", notmuch_tags_get(tags));
 		first_tag = 0;
 	    }
 
-	    fputs (format->tag_end, stdout);
+	    if(format != &format_text) {
+		format->end_array(thread, stdout);
+		format->end_attribute(thread, stdout);
+		format->end_object(thread, stdout);
+	    } else {
+		printf(")\n");
+	    }
 
-	    fputs (format->item_end, stdout);
 	}
 
 	first_thread = 0;
@@ -276,10 +385,10 @@ 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 != &format_text) {
+	format->end_array(threads, stdout);
+	fputs("\n", stdout);
+    }
 
     return 0;
 }
@@ -307,7 +416,8 @@ do_search_messages (const search_format_t *format,
     if (messages == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if(format != &format_text)
+	format->start_array(messages, stdout);
 
     for (i = 0;
 	 notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);
@@ -325,11 +435,15 @@ 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);
+		if (! first_message) {
+		    if(format != &format_text)
+			format->array_item_separator(message, stdout);
+		}
 
-		format->item_id (message, "",
-				 notmuch_filenames_get (filenames));
+		if(format != &format_text)
+		    format->string(message, stdout, notmuch_filenames_get (filenames));
+		else
+		    printf("%s\n", notmuch_filenames_get (filenames));
 
 		first_message = 0;
 	    }
@@ -337,11 +451,15 @@ do_search_messages (const search_format_t *format,
 	    notmuch_filenames_destroy( filenames );
 
 	} else { /* output == OUTPUT_MESSAGES */
-	    if (! first_message)
-		fputs (format->item_sep, stdout);
+	    if (! first_message) {
+	    if(format != &format_text)
+		format->array_item_separator(message, stdout);
+	    }
 
-	    format->item_id (message, "id:",
-			     notmuch_message_get_message_id (message));
+	    if(format != &format_text)
+		format->string(message, stdout, notmuch_message_get_message_id (message));
+	    else /* text format */
+		printf("id:%s\n", notmuch_message_get_message_id (message));
 	    first_message = 0;
 	}
 
@@ -350,11 +468,11 @@ 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 != &format_text) {
+	format->end_array(messages, stdout);
+	fputs("\n", stdout);
+    }
+    
     return 0;
 }
 
@@ -381,7 +499,8 @@ do_search_tags (notmuch_database_t *notmuch,
     if (tags == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if(format != &format_text)
+	format->start_array(tags, stdout);
 
     for (;
 	 notmuch_tags_valid (tags);
@@ -389,10 +508,15 @@ do_search_tags (notmuch_database_t *notmuch,
     {
 	tag = notmuch_tags_get (tags);
 
-	if (! first_tag)
-	    fputs (format->item_sep, stdout);
+	if (! first_tag) {
+	    if(format != &format_text)
+		format->array_item_separator(tag, stdout);
+	}
 
-	format->item_id (tags, "", tag);
+	if(format != &format_text)
+	    format->string(tags, stdout, tag);
+	else
+	    printf("%s\n", tag);
 
 	first_tag = 0;
     }
@@ -402,10 +526,8 @@ 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 != &format_text)
+	format->end_array(tags, stdout);
 
     return 0;
 }
@@ -427,7 +549,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     size_t auto_exclude_tags_length;
     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[] = {
@@ -437,6 +559,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
 	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
+				  { "sexp", NOTMUCH_FORMAT_SEXP },
 				  { "text", NOTMUCH_FORMAT_TEXT },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
@@ -464,6 +587,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     case NOTMUCH_FORMAT_JSON:
 	format = &format_json;
 	break;
+    case NOTMUCH_FORMAT_SEXP:
+	format = &format_sexp;
     }
 
     config = notmuch_config_open (ctx, NULL, NULL);
-- 
1.7.7.3

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Austin Clements]

Quoth Peter Feigl on Jan 21 at 10:16 pm:
> The output routines have been rewritten so that logical structure
> (objects with key/value pairs, arrays, strings and numbers) are
> written instead of ad-hoc printfs. This allows for easier adaptation
> of other output formats, as only the routines that start/end an object
> etc. have to be rewritten. The logic is the same for all formats.
> The default text output is handled differently, special cases are
> inserted at the proper places, as it differs too much from the
> structured output.

I think this is a great idea and I'm a fan of having an S-expression
format, but I also think there's a much easier and more general way to
structure this.

In particular, I don't think you should hijack search_format, since
you'll wind up repeating most of this work for anything else that
needs to output structured data (notmuch show, at least).  Rather, I'd
suggest creating a general "structure printer" struct that isn't tied
to search.  You've essentially already done this, you just called it
search_format.  Then, leave the existing format callbacks in place,
just use this new API instead of lots of printf calls.

What about all of those annoying {tag,item}_{start,sep,end} fields?  I
think you can simultaneously get rid of those *and* simplify the
structure printer API.  If the structure printer is allowed to keep a
little state, it can automatically insert separators.  With a little
nesting state, it could even insert terminators by just saying "pop me
to this level".  This could probably be better, but I'm imagining
something like

struct sprinter *
new_json_sprinter (const void *ctx, FILE *stream);
struct sprinter *
new_sexp_sprinter (const void *ctx, FILE *stream);

/* Start a map (a JSON object or a S-expression alist/plist/whatever)
 * and return the nesting level of the map. */
int
sprinter_map (struct sprinter *sp);
/* Start a list (aka array) and return the nesting level of the list. */
int
sprinter_list (struct sprinter *sp);

/* Close maps and lists until reaching level. */
void
sprinter_pop (struct sprinter *sp, int level);

void
sprinter_map_key (struct sprinter *sp, const char *key);
void
sprinter_number (struct sprinter *sp, int val);
void
sprinter_string (struct sprinter *sp, const char *val);
void
sprinter_bool (struct sprinter *sp, notmuch_bool_t val);

and that's it.  This would also subsume your format_attribute_*
helpers.

Unfortunately, it's a pain to pass things like a structure printer
object around formatters (too bad notmuch isn't written in C++, eh?).
I think it's better to address this than to structure around it.
Probably the simplest thing to do is to make a struct for formatter
state and pass that in to the callbacks.  You could also more
completely emulate classes, but that would probably be overkill for
this.

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

On Sat, 21 Jan 2012 22:16:08 +0100, Peter Feigl <craven@gmx.net> wrote:
> The output routines have been rewritten so that logical structure
> (objects with key/value pairs, arrays, strings and numbers) are
> written instead of ad-hoc printfs. This allows for easier adaptation
> of other output formats, as only the routines that start/end an object
> etc. have to be rewritten. The logic is the same for all formats.
> The default text output is handled differently, special cases are
> inserted at the proper places, as it differs too much from the
> structured output.

Hi, Peter.  Thanks for the contribution.

There are a lot of changes in this patch so I think you need to think
about how you can break this up into multiple smaller and more atomic
patches.  In particular, the addition of the sexp output format needs to
definitely be in a separate patch from the restructuring of the output
formatting.  You also don't mention anywhere in the commit log that
you've added this new output format.  You'll also need to include
documentation and test suite updates.

Thanks.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Peter Feigl]

[-- Attachment #1: Type: text/plain, Size: 3990 bytes --]

On Sat, 21 Jan 2012 17:04:07 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Peter Feigl on Jan 21 at 10:16 pm:
> I think this is a great idea and I'm a fan of having an S-expression
> format, but I also think there's a much easier and more general way to
> structure this.
> 
> In particular, I don't think you should hijack search_format, since
> you'll wind up repeating most of this work for anything else that
> needs to output structured data (notmuch show, at least).  Rather, I'd
> suggest creating a general "structure printer" struct that isn't tied
> to search.  You've essentially already done this, you just called it
> search_format.  Then, leave the existing format callbacks in place,
> just use this new API instead of lots of printf calls.

I'm sorry I haven't been more clear about this, the intention was all
along to check whether this would be ok in notmuch-search, and if it got
accepted there, to factor it out into a separate file and then use it in
notmuch-show and notmuch-reply. There's nothing in the printer (except
for the name of the struct) that ties it to search.

> What about all of those annoying {tag,item}_{start,sep,end} fields?  I
> think you can simultaneously get rid of those *and* simplify the
> structure printer API.  If the structure printer is allowed to keep a
> little state, it can automatically insert separators.  With a little
> nesting state, it could even insert terminators by just saying "pop me
> to this level".  This could probably be better, but I'm imagining
> something like

I agree, however this is complicated by the fact that there are
additional restrictions on the actually printed code: newlines should be
placed at strategic locations to improve parsability, which could be
hard to decide in the printer alone without support from the logic that
drives it.

> struct sprinter *
> new_json_sprinter (const void *ctx, FILE *stream);
> struct sprinter *
> new_sexp_sprinter (const void *ctx, FILE *stream);
> 
> /* Start a map (a JSON object or a S-expression alist/plist/whatever)
>  * and return the nesting level of the map. */
> int
> sprinter_map (struct sprinter *sp);
> /* Start a list (aka array) and return the nesting level of the list. */
> int
> sprinter_list (struct sprinter *sp);
> 
> /* Close maps and lists until reaching level. */
> void
> sprinter_pop (struct sprinter *sp, int level);
> 
> void
> sprinter_map_key (struct sprinter *sp, const char *key);
> void
> sprinter_number (struct sprinter *sp, int val);
> void
> sprinter_string (struct sprinter *sp, const char *val);
> void
> sprinter_bool (struct sprinter *sp, notmuch_bool_t val);
> 
> and that's it.  This would also subsume your format_attribute_*
> helpers.
> 
> Unfortunately, it's a pain to pass things like a structure printer
> object around formatters (too bad notmuch isn't written in C++, eh?).
> I think it's better to address this than to structure around it.
> Probably the simplest thing to do is to make a struct for formatter
> state and pass that in to the callbacks.  You could also more
> completely emulate classes, but that would probably be overkill for
> this.

I believe this approach is similar to the one I've implemented (though
probably higher level, not so many details explicitly written into the
formatting code). I would suggest trying to get any sort of structured
formatters (whether more like your suggestions or like the thing I
implemented doesn't matter so much) into the main codebase, and then
refactoring the other parts to use it. I've thought about providing a
single patch to all of notmuch that accomplishes this, but I've deemed
it too large and complicated to be accepted, I thought limiting it to
notmuch-search would be a way to get it set up, so that it could be
expanded to the other parts later.

Thanks for the comments, I'll keep thinking about your design, a very
interesting idea!

Peter

[-- Attachment #2: Type: application/pgp-signature, Size: 274 bytes --]

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Peter Feigl]

On Sat, 21 Jan 2012 15:12:57 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Sat, 21 Jan 2012 22:16:08 +0100, Peter Feigl <craven@gmx.net> wrote:
> > The output routines have been rewritten so that logical structure
> > (objects with key/value pairs, arrays, strings and numbers) are
> > written instead of ad-hoc printfs. This allows for easier adaptation
> > of other output formats, as only the routines that start/end an object
> > etc. have to be rewritten. The logic is the same for all formats.
> > The default text output is handled differently, special cases are
> > inserted at the proper places, as it differs too much from the
> > structured output.
> 
> Hi, Peter.  Thanks for the contribution.
> 
> There are a lot of changes in this patch so I think you need to think
> about how you can break this up into multiple smaller and more atomic
> patches.  In particular, the addition of the sexp output format needs to
> definitely be in a separate patch from the restructuring of the output
> formatting.  You also don't mention anywhere in the commit log that
> you've added this new output format.  You'll also need to include
> documentation and test suite updates.

I'm sorry I forgot to mention that, it was mainly meant as a way to show
that this is easily possible (i.e. that the formatting is decoupled from
the logic, so that additional and different formats can be added without
influencing the printing logic). It'd be easy to split this up. What
kind of documentation should I include? 
The test suite should work fine, *if* it compares EXPECTED and OUTPUT
not character-by-character, but rather by pretty-printing both the
expected and the actual outputs by some JSON pretty-printer (like python
-mjson.tool). I can of course provide additional test-cases for
--format=sexp.

How should I proceed on this? Re-submit the patch with the sexp-support
removed and only JSON updated?

Thanks!

Peter

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Austin Clements]

Quoth Peter Feigl on Jan 22 at 12:17 am:
> On Sat, 21 Jan 2012 17:04:07 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > Quoth Peter Feigl on Jan 21 at 10:16 pm:
> > I think this is a great idea and I'm a fan of having an S-expression
> > format, but I also think there's a much easier and more general way to
> > structure this.
> > 
> > In particular, I don't think you should hijack search_format, since
> > you'll wind up repeating most of this work for anything else that
> > needs to output structured data (notmuch show, at least).  Rather, I'd
> > suggest creating a general "structure printer" struct that isn't tied
> > to search.  You've essentially already done this, you just called it
> > search_format.  Then, leave the existing format callbacks in place,
> > just use this new API instead of lots of printf calls.
> 
> I'm sorry I haven't been more clear about this, the intention was all
> along to check whether this would be ok in notmuch-search, and if it got
> accepted there, to factor it out into a separate file and then use it in
> notmuch-show and notmuch-reply. There's nothing in the printer (except
> for the name of the struct) that ties it to search.

Great!  However, it seems counterproductive to put all of these
structures and functions into search only to then move them somewhere
else.  Wouldn't it make more sense to introduce the generic structure
printer in a first patch, then refactor the existing code to use it in
a second (or maybe more) patch in a series?

> > What about all of those annoying {tag,item}_{start,sep,end} fields?  I
> > think you can simultaneously get rid of those *and* simplify the
> > structure printer API.  If the structure printer is allowed to keep a
> > little state, it can automatically insert separators.  With a little
> > nesting state, it could even insert terminators by just saying "pop me
> > to this level".  This could probably be better, but I'm imagining
> > something like
> 
> I agree, however this is complicated by the fact that there are
> additional restrictions on the actually printed code: newlines should be
> placed at strategic locations to improve parsability, which could be
> hard to decide in the printer alone without support from the logic that
> drives it.

True, but that support is easy to add and, I would argue, the exact
implementation of framing *should* be up to the printer (though
obviously where to place the framing should be up to the caller).
Following the general structure of the API I was proposing, you could
add a function like

/* Print a framing break that is easy to detect in a parser. */
void
sprinter_break (struct sprinter *sp);

that for JSON and S-expressions could just print a newline.  Or, for
JSON, if we want separators to appear before the newline (which they
don't have to; we can choose our framing however we want), it could
simply record that a newline should be printed after the next
separator or terminator.

> > struct sprinter *
> > new_json_sprinter (const void *ctx, FILE *stream);
> > struct sprinter *
> > new_sexp_sprinter (const void *ctx, FILE *stream);
> > 
> > /* Start a map (a JSON object or a S-expression alist/plist/whatever)
> >  * and return the nesting level of the map. */
> > int
> > sprinter_map (struct sprinter *sp);
> > /* Start a list (aka array) and return the nesting level of the list. */
> > int
> > sprinter_list (struct sprinter *sp);
> > 
> > /* Close maps and lists until reaching level. */
> > void
> > sprinter_pop (struct sprinter *sp, int level);
> > 
> > void
> > sprinter_map_key (struct sprinter *sp, const char *key);
> > void
> > sprinter_number (struct sprinter *sp, int val);
> > void
> > sprinter_string (struct sprinter *sp, const char *val);
> > void
> > sprinter_bool (struct sprinter *sp, notmuch_bool_t val);
> > 
> > and that's it.  This would also subsume your format_attribute_*
> > helpers.
> > 
> > Unfortunately, it's a pain to pass things like a structure printer
> > object around formatters (too bad notmuch isn't written in C++, eh?).
> > I think it's better to address this than to structure around it.
> > Probably the simplest thing to do is to make a struct for formatter
> > state and pass that in to the callbacks.  You could also more
> > completely emulate classes, but that would probably be overkill for
> > this.
> 
> I believe this approach is similar to the one I've implemented (though
> probably higher level, not so many details explicitly written into the
> formatting code). I would suggest trying to get any sort of structured

*nods* It was definitely inspired by yours.  The complexity has to go
somewhere, and in the long run it seems it would be better to isolate
it in the printer than to deal with it in every caller.  My ulterior
motive was to maintain roughly the existing and extensible callback
structure while eliminating the need for the various start/sep/end
fields, which would have to differ between the formats and would
otherwise duplicate knowledge that should be isolated to a structure
printer.

> formatters (whether more like your suggestions or like the thing I
> implemented doesn't matter so much) into the main codebase, and then
> refactoring the other parts to use it. I've thought about providing a
> single patch to all of notmuch that accomplishes this, but I've deemed
> it too large and complicated to be accepted, I thought limiting it to
> notmuch-search would be a way to get it set up, so that it could be
> expanded to the other parts later.

I've found that smaller patches are much more likely to get reviewed
on the notmuch list, even if they're part of a series that adds up to
a big change.

> Thanks for the comments, I'll keep thinking about your design, a very
> interesting idea!
> 
> Peter

Re: [PATCH] rewriting notmuch-search for structured output to make other output formats easier

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Sun, 22 Jan 2012 00:21:37 +0100, "Peter Feigl" <craven@gmx.net> wrote:
> What kind of documentation should I include?

Update the man page to describe the new format and command line options.

> The test suite should work fine, *if* it compares EXPECTED and OUTPUT
> not character-by-character, but rather by pretty-printing both the
> expected and the actual outputs by some JSON pretty-printer (like python
> -mjson.tool). I can of course provide additional test-cases for
> --format=sexp.

I was referring specifically to new tests for the new output format.
The test suite changes should include only additions, since as you point
out, the internal restructuring shouldn't affect any existing tests.

> How should I proceed on this? Re-submit the patch with the sexp-support
> removed and only JSON updated?

I think you should primarily work on addressing Austin's issues
regarding the output formatters first, being careful to try to make more
small atomic patches.  Then once that's done make a new series of
patches, depending on the new formatter patches, that adds the new
output format.

As Austin points out, more smaller patches that are narrowly focused are
much easier to review, even if there ends up being more changes in the
end.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]