From: Mark Walters <markwalters1009@gmail.com>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>
Subject: Re: [PATCH 1/2] cli: make --entire-thread=false work for format=json.
In-Reply-To:
 <CAMoJFUuXmLQR+awJnPKg8ZJAKbHbPAmSrEUVoFj_=dZ76WiKzw@mail.gmail.com>
References: <1334077496-9172-1-git-send-email-markwalters1009@gmail.com>
	<1334077496-9172-2-git-send-email-markwalters1009@gmail.com>
	<CAMoJFUuXmLQR+awJnPKg8ZJAKbHbPAmSrEUVoFj_=dZ76WiKzw@mail.gmail.com>
User-Agent: Notmuch/0.12+101~g43fd4f1 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 10 Apr 2012 21:52:58 +0100
Message-ID: <87zkajux5h.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
Precedence: list

On Tue, 10 Apr 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Hi Mark,
>
> This looks good to me, but I haven't tested it. It's probably worth
> adding a test for the new functionality.

Yes I agree. I could add a test to the json tests, or I could update the
pair of test patches at
id:"1332171061-27983-1-git-send-email-markwalters1009@gmail.com" as
there do not seem to be any tests for --entire-thread or not for the
other formats either. What do you think?

> One style issue below, which is a matter of taste and I'll defer to
> others if they disagree:
>
> On Tue, Apr 10, 2012 at 11:04, Mark Walters <markwalters1009@gmail.com> w=
rote:
>> @@ -895,10 +905,11 @@ show_messages (void *ctx,
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (status && !res)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0res =3D status;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0next_indent =3D indent + 1;
>> + =C2=A0 =C2=A0 =C2=A0 } else
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 status =3D show_null_message (forma=
t);
>
> I accept, but don't particularly like, the notmuch style of omitting
> braces where they aren't required. However, an else with a brace on
> only one side just looks weird. If they're like this everywhere else
> then I guess it's best to be consistent, but to me it's a lot more
> readable as } else {.
>
> As I said above, I'll defer to others' judgement here, just thought it
> was worth pointing out.

Yes I will fix this in the next version (I recall Jani agreed with you
so I will take that as preferred unless someone says otherwise.)

thanks

Mark