Date: Thu, 12 Jul 2012 16:29:34 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: craven@gmx.net
Subject: Re: [PATCH v4 1/3] Add support for structured output formatters.
Message-ID: <20120712202934.GG7332@mit.edu>
References: <87d34hsdx8.fsf@awakening.csail.mit.edu>
	<1342079004-5300-1-git-send-email-craven@gmx.net>
	<1342079004-5300-2-git-send-email-craven@gmx.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1342079004-5300-2-git-send-email-craven@gmx.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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