12/49ec5297fa2f264f741a033f48aa5cc00cc468

   1 Return-Path: <amdragon@mit.edu>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id BD39A431FC4\r
   6         for <notmuch@notmuchmail.org>; Thu, 12 Jul 2012 13:29:39 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id HitGYQWVtJaO for <notmuch@notmuchmail.org>;\r
  16         Thu, 12 Jul 2012 13:29:38 -0700 (PDT)\r
  17 Received: from dmz-mailsec-scanner-5.mit.edu (DMZ-MAILSEC-SCANNER-5.MIT.EDU\r
  18         [18.7.68.34])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 00F45431FAF\r
  20         for <notmuch@notmuchmail.org>; Thu, 12 Jul 2012 13:29:37 -0700 (PDT)\r
  21 X-AuditID: 12074422-b7f1f6d00000090b-e9-4fff33b1be8c\r
  22 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36])\r
  23         by dmz-mailsec-scanner-5.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id F6.03.02315.1B33FFF4; Thu, 12 Jul 2012 16:29:37 -0400 (EDT)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q6CKTaWK011245; \r
  27         Thu, 12 Jul 2012 16:29:37 -0400\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q6CKTYir001950\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Thu, 12 Jul 2012 16:29:35 -0400 (EDT)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1SpQ10-0004fY-J8; Thu, 12 Jul 2012 16:29:34 -0400\r
  37 Date: Thu, 12 Jul 2012 16:29:34 -0400\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: craven@gmx.net\r
  40 Subject: Re: [PATCH v4 1/3] Add support for structured output formatters.\r
  41 Message-ID: <20120712202934.GG7332@mit.edu>\r
  42 References: <87d34hsdx8.fsf@awakening.csail.mit.edu>\r
  43         <1342079004-5300-1-git-send-email-craven@gmx.net>\r
  44         <1342079004-5300-2-git-send-email-craven@gmx.net>\r
  45 MIME-Version: 1.0\r
  46 Content-Type: text/plain; charset=us-ascii\r
  47 Content-Disposition: inline\r
  48 In-Reply-To: <1342079004-5300-2-git-send-email-craven@gmx.net>\r
  49 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFmphleLIzCtJLcpLzFFi42IRYrdT0d1o/N/f4PN9GYu9De2MFtdvzmR2\r
  52         YPJYvGk/m8ezVbeYA5iiuGxSUnMyy1KL9O0SuDJ+bnMt+KFT8WfmG+YGxvXKXYycHBICJhJf\r
  53         t/9hg7DFJC7cWw9kc3EICexjlLjydAcrhLOBUWLGl0ZGCOckk8T/jd+hMksYJZo2/GAG6WcR\r
  54         UJVYumES2Cw2AQ2JbfuXM4LYIgJCEpO+vGIBsZkFpCW+/W5mArGFBTwlTp1uBYvzCmhLTJ/X\r
  55         xw4xdA6jxMTOR6wQCUGJkzOfQDVrSdz49xKomQNs0PJ/HCBhTgE7iVPT/4PtEhVQkZhychvb\r
  56         BEahWUi6ZyHpnoXQvYCReRWjbEpulW5uYmZOcWqybnFyYl5eapGuqV5uZoleakrpJkZwYLso\r
  57         7WD8eVDpEKMAB6MSD+/OdX/9hVgTy4orcw8xSnIwKYnyagDjQogvKT+lMiOxOCO+qDQntfgQ\r
  58         owQHs5IIb5YEUI43JbGyKrUoHyYlzcGiJM57LeWmv5BAemJJanZqakFqEUxWhoNDSYJXFWSo\r
  59         YFFqempFWmZOCUKaiYMTZDgP0PBAkBre4oLE3OLMdIj8KUZFKXFecZCEAEgiozQPrheWeF4x\r
  60         igO9IgzRzgNMWnDdr4AGMwENnvXzH8jgkkSElFQDo1YEn8IKh9+sEUbV1/ebrOrcFLLGR9Vj\r
  61         c4HyOwFNh9MH+DseC6lGBuRx2b+b0mqz1fEw81busMCqgHvXFGdd+89+09nt/MPHuvMmVU/S\r
  62         eLbHw7c7Z9NBM9/oGS8Tr4ndlXqRujTx+7fyDf0fV5m/dTM3vmzKaOR048IJkTlvyljWSRje\r
  63         KTLuUWIpzkg01GIuKk4EAB0RSF0XAwAA\r
  64 Cc: notmuch@notmuchmail.org\r
  65 X-BeenThere: notmuch@notmuchmail.org\r
  66 X-Mailman-Version: 2.1.13\r
  67 Precedence: list\r
  68 List-Id: "Use and development of the notmuch mail system."\r
  69         <notmuch.notmuchmail.org>\r
  70 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  72 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  73 List-Post: <mailto:notmuch@notmuchmail.org>\r
  74 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  75 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  76         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  77 X-List-Received-Date: Thu, 12 Jul 2012 20:29:39 -0000\r
  78 \r
  79 Quoth craven@gmx.net on Jul 12 at  9:43 am:\r
  80 > This patch adds a new type sprinter_t, which is used for structured\r
  81 > formatting, e.g. JSON or S-Expressions. The structure printer is the\r
  82 > code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).\r
  83 > \r
  84 > The structure printer contains the following function pointers:\r
  85 > \r
  86 > /* start a new map/dictionary structure.\r
  87 >  */\r
  88 > void (*begin_map) (struct sprinter *);\r
  89 > \r
  90 > /* start a new list/array structure\r
  91 >  */\r
  92 > void (*begin_list) (struct sprinter *);\r
  93 > \r
  94 > /* end the last opened list or map structure\r
  95 >  */\r
  96 > void (*end) (struct sprinter *);\r
  97 > \r
  98 > /* print one string/integer/boolean/null element (possibly inside a\r
  99 >  * list or map\r
 100 >  */\r
 101 > void (*string) (struct sprinter *, const char *);\r
 102 > void (*integer) (struct sprinter *, int);\r
 103 > void (*boolean) (struct sprinter *, notmuch_bool_t);\r
 104 > void (*null) (struct sprinter *);\r
 105 > \r
 106 > /* print the key of a map's key/value pair.\r
 107 >  */\r
 108 > void (*map_key) (struct sprinter *, const char *);\r
 109 > \r
 110 > /* print a frame delimiter (such as a newline or extra whitespace)\r
 111 >  */\r
 112 > void (*frame) (struct sprinter *);\r
 113 > \r
 114 > The printer can (and should) use internal state to insert delimiters and\r
 115 > syntax at the correct places.\r
 116 > \r
 117 > Example:\r
 118 > \r
 119 > format->begin_map(format);\r
 120 > format->map_key(format, "foo");\r
 121 > format->begin_list(format);\r
 122 > format->integer(format, 1);\r
 123 > format->integer(format, 2);\r
 124 > format->integer(format, 3);\r
 125 > format->end(format);\r
 126 > format->map_key(format, "bar");\r
 127 > format->begin_map(format);\r
 128 > format->map_key(format, "baaz");\r
 129 > format->string(format, "hello world");\r
 130 > format->end(format);\r
 131 > format->end(format);\r
 132 > \r
 133 > would output JSON as follows:\r
 134 > \r
 135 > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}\r
 136 > ---\r
 137 >  sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\r
 138 >  1 file changed, 49 insertions(+)\r
 139 >  create mode 100644 sprinter.h\r
 140 > \r
 141 > diff --git a/sprinter.h b/sprinter.h\r
 142 > new file mode 100644\r
 143 > index 0000000..1dad9a0\r
 144 > --- /dev/null\r
 145 > +++ b/sprinter.h\r
 146 > @@ -0,0 +1,49 @@\r
 147 > +#ifndef NOTMUCH_SPRINTER_H\r
 148 > +#define NOTMUCH_SPRINTER_H\r
 149 > +\r
 150 > +/* for notmuch_bool_t */\r
 151 \r
 152 Style/consistency nit: all of the comments should start with a capital\r
 153 letter and anything that's a sentence should end with a period (some\r
 154 of your comments do, some of them don't).\r
 155 \r
 156 > +#include "notmuch-client.h"\r
 157 > +\r
 158 > +/* Structure printer interface */\r
 159 > +typedef struct sprinter\r
 160 > +{\r
 161 > +    /* start a new map/dictionary structure.\r
 162 \r
 163 This should probably mention how other functions should be called\r
 164 within the map structure.  Perhaps,\r
 165 \r
 166 /* Start a new map/dictionary structure.  This should be followed by a\r
 167  * sequence of alternating calls to map_key and one of the value\r
 168  * printing functions until the map is ended.\r
 169  */\r
 170 \r
 171 (You had a comment to this effect in one of your earlier patches.)\r
 172 \r
 173 > +     */\r
 174 > +    void (*begin_map) (struct sprinter *);\r
 175 > +\r
 176 > +    /* start a new list/array structure\r
 177 > +     */\r
 178 > +    void (*begin_list) (struct sprinter *);\r
 179 > +\r
 180 > +    /* end the last opened list or map structure\r
 181 > +     */\r
 182 > +    void (*end) (struct sprinter *);\r
 183 > +\r
 184 > +    /* print one string/integer/boolean/null element (possibly inside a\r
 185 > +     * list or map\r
 186 \r
 187 Missing close paren.\r
 188 \r
 189 > +     */\r
 190 > +    void (*string) (struct sprinter *, const char *);\r
 191 > +    void (*integer) (struct sprinter *, int);\r
 192 > +    void (*boolean) (struct sprinter *, notmuch_bool_t);\r
 193 > +    void (*null) (struct sprinter *);\r
 194 > +\r
 195 > +    /* print the key of a map's key/value pair.\r
 196 > +     */\r
 197 > +    void (*map_key) (struct sprinter *, const char *);\r
 198 > +\r
 199 > +    /* print a frame delimiter (such as a newline or extra whitespace)\r
 200 > +     */\r
 201 > +    void (*frame) (struct sprinter *);\r
 202 \r
 203 I wonder if frame is still necessary.  At the time, I was thinking\r
 204 that we would use embedded newline framing to do streaming JSON\r
 205 parsing, but I wound up going with a general incremental JSON parser,\r
 206 eliminating the need for framing.\r
 207 \r
 208 It's probably still useful to have a function that inserts a newline\r
 209 purely for readability/debuggability purposes.  In practice, the\r
 210 implementation would be the same as frame (though if it's for\r
 211 readability, maybe you'd want to print the comma before the newline\r
 212 instead of after), but since the intent would be different, it would\r
 213 need different documentation and maybe a different name.  "Frame"\r
 214 isn't a bad name for either intent.  We can't use "break".\r
 215 "Separator" (or just "sep")?  "Space"?  "Split"?  The comment could be\r
 216 something like\r
 217 \r
 218 /* Insert a separator for improved readability without affecting the\r
 219  * abstract syntax of the structure being printed.  For JSON, this is\r
 220  * simply a line break.\r
 221  */\r
 222 \r
 223 > +} sprinter_t;\r
 224 > +\r
 225 > +/* Create a new structure printer that emits JSON */\r
 226 > +struct sprinter *\r
 227 > +sprinter_json_new(const void *ctx, FILE *stream);\r
 228 \r
 229 This should probably be called sprinter_json_create to be consistent\r
 230 with the naming of other constructors in notmuch.\r
 231 \r
 232 Also, missing space between the function name and parameter list\r
 233 (sorry, my fault).\r
 234 \r
 235 > +\r
 236 > +/* A dummy structure printer that signifies that standard text output is\r
 237 > + * to be used instead of any structured format.\r
 238 > + */\r
 239 > +struct sprinter *\r
 240 > +sprinter_text;\r
 241 \r
 242 It looks like you never assign anything to this pointer, so all of\r
 243 your tests against sprinter_text are equivalent to just checking for a\r
 244 NULL pointer.  You could either just use NULL as the designated value\r
 245 for the text format, or you could use this global variable, but make\r
 246 it a struct sprinter instead of a struct sprinter *.\r
 247 \r
 248 Also, this can probably be a static declaration in notmuch-search.c.\r
 249 \r
 250 > +\r
 251 > +#endif // NOTMUCH_SPRINTER_H\r