e6/6ed2f420bc88f923a18b257d9af6390677eb1b

   1 Return-Path: <cworth@cworth.org>\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 6C26D431FBF\r
   6         for <notmuch@notmuchmail.org>; Tue, 23 Feb 2010 11:56:13 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -3.254\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-3.254 tagged_above=-999 required=5\r
  12         tests=[ALL_TRUSTED=-1.8, AWL=1.145, BAYES_00=-2.599] autolearn=ham\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 rghDn-3eeMpK; Tue, 23 Feb 2010 11:56:12 -0800 (PST)\r
  16 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  17         by olra.theworths.org (Postfix) with ESMTP id 9D3AE431FAE;\r
  18         Tue, 23 Feb 2010 11:56:12 -0800 (PST)\r
  19 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  20         id E47D025427B; Tue, 23 Feb 2010 11:56:10 -0800 (PST)\r
  21 From: Carl Worth <cworth@cworth.org>\r
  22 To: david@tethera.net, notmuch@notmuchmail.org\r
  23 In-Reply-To: <1261141195-5469-1-git-send-email-david@tethera.net>\r
  24 References: <1261114167-sup-8228@lisa>\r
  25         <1261141195-5469-1-git-send-email-david@tethera.net>\r
  26 Date: Tue, 23 Feb 2010 11:56:10 -0800\r
  27 Message-ID: <878wajwwth.fsf@yoom.home.cworth.org>\r
  28 MIME-Version: 1.0\r
  29 Content-Type: multipart/signed; boundary="=-=-=";\r
  30         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  31 Subject: Re: [notmuch] [PATCH] Add an "--output=(json|text|)" command-line\r
  32  option to both notmuch-search and notmuch-show.\r
  33 X-BeenThere: notmuch@notmuchmail.org\r
  34 X-Mailman-Version: 2.1.13\r
  35 Precedence: list\r
  36 List-Id: "Use and development of the notmuch mail system."\r
  37         <notmuch.notmuchmail.org>\r
  38 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  39         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  40 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  41 List-Post: <mailto:notmuch@notmuchmail.org>\r
  42 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  43 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  44         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  45 X-List-Received-Date: Tue, 23 Feb 2010 19:56:13 -0000\r
  46 \r
  47 --=-=-=\r
  48 Content-Type: text/plain; charset=utf-8\r
  49 Content-Transfer-Encoding: quoted-printable\r
  50 \r
  51 On Fri, 18 Dec 2009 08:59:55 -0400, david@tethera.net wrote:\r
  52 > From: Scott Robinson <scott@quadhome.com>\r
  53 \r
  54 Hi Scott (and David),\r
  55 \r
  56 I'm finally getting around to looking closely at the JSON patches\r
  57 (hurrah!). Here are some comments:\r
  58 \r
  59 > In the case of notmuch-show, "--output=3Djson" also implies\r
  60 > "--entire-thread" as the thread structure is implicit in the emitted\r
  61 > document tree.\r
  62 \r
  63 I think we've all agreed to use --output for selecting what to print and\r
  64 to instead use --format for how to format it. I also think David's got a\r
  65 patch to change that, but if we have to change the current patch anyway,\r
  66 we might change that at the same time.\r
  67 \r
  68 > --- /dev/null\r
  69 > +++ b/json.c\r
  70 > @@ -0,0 +1,73 @@\r
  71 > +/* notmuch - Not much of an email program, (just index and search)\r
  72 > + *\r
  73 > + * Copyright =C2=A9 2009 Carl Worth\r
  74 > + * Copyright =C2=A9 2009 Keith Packard\r
  75 \r
  76 I know I didn't contribute any code to this file, so my name shouldn't\r
  77 be here. Scott, I imagine you have made some non-trivial contributions\r
  78 so your name (or suitable copyright-claiming entity) should be here.\r
  79 \r
  80 > + * Authors: Carl Worth <cworth@cworth.org>\r
  81 > + *       Keith Packard <keithp@keithp.com>\r
  82 \r
  83 Same thing here.\r
  84 \r
  85 > +/*\r
  86 > + * json_quote_str derived from cJSON's print_string_ptr,\r
  87 > + * Copyright (c) 2009 Dave Gamble\r
  88 > + */\r
  89 \r
  90 Thanks for attributing the source here, but let's please keep all the\r
  91 copyright statements up at the top of the file.\r
  92 \r
  93 It would still be reasonable to have a comment at this point that this\r
  94 particular code came from Dave Gamble and cJSON. But it should also\r
  95 mention the license under which the code is being integrated.\r
  96 \r
  97 I suggest leaving the notmuch-standar GPLv3+ blurb at the top of the\r
  98 file, but then quoting the license itself of the external code, (it's a\r
  99 short MIT-style license, right?).\r
 100 \r
 101 > +char *\r
 102 > +json_quote_str(const void *ctx, const char *str)\r
 103 \r
 104 It would be nice to have a little comment here describing what the\r
 105 function does, (what characters get quoted and how, that the return\r
 106 value is talloced with 'ctx' as a parent, and perhaps a pointer to the\r
 107 appropriate JSON reference/specification).\r
 108 \r
 109 But I definitely like this nice little function as opposed to some JSON\r
 110 library. Thanks!\r
 111 \r
 112 \r
 113 > +typedef struct search_format {\r
 114 > +    const char *results_start;\r
 115 > +    const char *thread_start;\r
 116 > +    void (*thread) (const void *ctx,\r
 117 > +                 const char *id,\r
 118 > +                 const time_t date,\r
 119 > +                 const int matched,\r
 120 > +                 const int total,\r
 121 > +                 const char *authors,\r
 122 > +                 const char *subject);\r
 123 ...\r
 124 \r
 125 Definitely missing at least a quick comment for internal documentation\r
 126 here as well. But I do like the way this works.\r
 127 \r
 128 > --- a/notmuch.c\r
 129 > +++ b/notmuch.c\r
 130 > @@ -162,6 +162,11 @@ command_t commands[] =3D {\r
 131 >        "\n"\r
 132 >        "\t\tSupported options for search include:\n"\r
 133 >        "\n"\r
 134 > +      "\t\t--output=3D(json|text)\n"\r
 135 > +      "\n"\r
 136 > +      "\t\t\tPresents the results in either JSON or plain-text\n"\r
 137 > +      "\t\t\tformat, which is the default.\n"\r
 138 > +      "\n"\r
 139 \r
 140 Thanks for adding the documentation here. But please add to notmuch.1 as\r
 141 well.\r
 142 \r
 143 This all looks really great. And I can't wait to apply it and play with\r
 144 it more.\r
 145 \r
 146 =2DCarl\r
 147 \r
 148 --=-=-=\r
 149 Content-Type: application/pgp-signature\r
 150 \r
 151 -----BEGIN PGP SIGNATURE-----\r
 152 Version: GnuPG v1.4.10 (GNU/Linux)\r
 153 \r
 154 iD8DBQFLhDLa6JDdNq8qSWgRArdUAJ932a5kxn2n91gFuSB8batyaoWnAwCeJirW\r
 155 Cj+6sjGiw9F1zGV9hEd9+UA=\r
 156 =imco\r
 157 -----END PGP SIGNATURE-----\r
 158 --=-=-=--\r