On Fri, 18 Dec 2009 08:59:55 -0400, david@tethera.net wrote: > From: Scott Robinson <scott@quadhome.com> Hi Scott (and David), I'm finally getting around to looking closely at the JSON patches (hurrah!). Here are some comments: > In the case of notmuch-show, "--output=json" also implies > "--entire-thread" as the thread structure is implicit in the emitted > document tree. I think we've all agreed to use --output for selecting what to print and to instead use --format for how to format it. I also think David's got a patch to change that, but if we have to change the current patch anyway, we might change that at the same time. > --- /dev/null > +++ b/json.c > @@ -0,0 +1,73 @@ > +/* notmuch - Not much of an email program, (just index and search) > + * > + * Copyright © 2009 Carl Worth > + * Copyright © 2009 Keith Packard I know I didn't contribute any code to this file, so my name shouldn't be here. Scott, I imagine you have made some non-trivial contributions so your name (or suitable copyright-claiming entity) should be here. > + * Authors: Carl Worth <cworth@cworth.org> > + * Keith Packard <keithp@keithp.com> Same thing here. > +/* > + * json_quote_str derived from cJSON's print_string_ptr, > + * Copyright (c) 2009 Dave Gamble > + */ Thanks for attributing the source here, but let's please keep all the copyright statements up at the top of the file. It would still be reasonable to have a comment at this point that this particular code came from Dave Gamble and cJSON. But it should also mention the license under which the code is being integrated. I suggest leaving the notmuch-standar GPLv3+ blurb at the top of the file, but then quoting the license itself of the external code, (it's a short MIT-style license, right?). > +char * > +json_quote_str(const void *ctx, const char *str) It would be nice to have a little comment here describing what the function does, (what characters get quoted and how, that the return value is talloced with 'ctx' as a parent, and perhaps a pointer to the appropriate JSON reference/specification). But I definitely like this nice little function as opposed to some JSON library. Thanks! > +typedef struct search_format { > + const char *results_start; > + const char *thread_start; > + void (*thread) (const void *ctx, > + const char *id, > + const time_t date, > + const int matched, > + const int total, > + const char *authors, > + const char *subject); ... Definitely missing at least a quick comment for internal documentation here as well. But I do like the way this works. > --- a/notmuch.c > +++ b/notmuch.c > @@ -162,6 +162,11 @@ command_t commands[] = { > "\n" > "\t\tSupported options for search include:\n" > "\n" > + "\t\t--output=(json|text)\n" > + "\n" > + "\t\t\tPresents the results in either JSON or plain-text\n" > + "\t\t\tformat, which is the default.\n" > + "\n" Thanks for adding the documentation here. But please add to notmuch.1 as well. This all looks really great. And I can't wait to apply it and play with it more. -Carl