Re: [PATCH v4 2/4] reply: Add a JSON reply format.

[Dmitry Kurochkin <dmitry.kurochkin@gmail.com>]

Hi Adam.

On Thu, 9 Feb 2012 21:27:29 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Hi Dmitry,
> 
> On Thu, Feb 9, 2012 at 00:22, Dmitry Kurochkin
> <dmitry.kurochkin@gmail.com> wrote:
> >> Reply now enforces that only one message is returned, as the semantics
> >> of replying to multiple messages are not wel-defined.
> >
> > s/wel/well/
> 
> Oops! git filter-branch to the rescue :-).
> 
> >> Show is modified such that --format=json no longer implies
> >> --entire-thread, as MUAs will use --format=json when constructing
> >> replies. The man page is updated to reflect this change.
> >
> > I did not look into details.  But I am surprised that user needs to call
> > notmuch show --format=json to make reply.  I would expect notmuch reply
> > to provide all required info (except for bodies).
> 
> I agree, it would be ideal to include the data from show --format=json
> in the reply JSON. I started down the path of implementing this, but
> realized it requires either copying quite a bit of code from show or
> factoring it out, and both options felt kind of dirty. I'd like show
> and reply to share a function that produces the JSON-formatted body of
> a message, but it doesn't feel right to expose the entire JSON format
> and all the functions that go with it from notmuch-show.c and put the
> structure and all the prototypes in notmuch-client.h.
> 

I see.

> Will Austin's show rewrite make this easier/cleaner? Or am I being too
> squeamish about moving code?
> 

It probably will.

> > Anyway, I think you should put this change in a separate patch.
> 
> Yeah, if I leave it as is it changes a default behavior, so a separate
> patch would probably be a good idea.
> 
> > Also, we clearly need a NEWS entry for it and user-customizable Emacs
> > variable changes.  Though it can be done after this series is pushed, I
> > guess.
> 
> I don't think there are any emacs customization changes here, unless
> we want to implement the notmuch mode config file that was discussed
> before to give message-citation-line-format and other things nice
> defaults. Or are you suggesting that there should be some new
> customization options? (One I can think of would be the list of
> preferred types for multipart/alternative display, which right now is
> hardcoded in notmuch-show.el.)
> 

No.  I thought there were some user-customizable variables that were
moved to notmuch-lib and renamed.  If that is correct, it would not hurt
to document these changes.

> > Thank you for this work, it is much appreciated.
> 
> Thanks for taking the time to review these patches! I think as a
> result of everyone's reviews I've pushed the series toward what I
> envisioned/wanted in the first place, rather than the kinda kludgey
> thing I did initially.
> 

No need to thank me for review, I did not do it :)

> Amusingly, I've started a new job since I wrote the original patch and
> no longer receive much HTML-only email, but I like this patch series
> enough to see it through anyway :-).

Congratulations with the new job :)

Regards,
  Dmitry