[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

Let notmuch-poll-script be a function as well as a string. Make default
value nil instead of an empty string, but allow "" for backwards
compatibility. Add a notmuch poll function to call "notmuch new" using the
configured notmuch-command.

This allows taking better advantage of the "notmuch new" hooks from emacs
without intermediate scripts.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 emacs/notmuch.el |   44 ++++++++++++++++++++++++++++++++------------
 1 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8936149..6c7e800 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -965,28 +965,48 @@ same relative position within the new buffer."
     (notmuch-search query oldest-first target-thread target-line continuation)
     (goto-char (point-min))))
 
-(defcustom notmuch-poll-script ""
-  "An external script to incorporate new mail into the notmuch database.
+(defcustom notmuch-poll-script nil
+  "A script or a function to incorporate new mail into the notmuch database.
 
-If this variable is non empty, then it should name a script to be
-invoked by `notmuch-search-poll-and-refresh-view' and
-`notmuch-hello-poll-and-update' (each have a default keybinding
-of 'G'). The script could do any of the following depending on
+This variable can be set to a function or the name of an external
+script to be invoked by `notmuch-search-poll-and-refresh-view'
+and `notmuch-hello-poll-and-update' (each have a default
+keybinding of 'G'). Set to nil to do nothing.
+
+The function or script could do any of the following depending on
 the user's needs:
 
 1. Invoke a program to transfer mail to the local mail store
 2. Invoke \"notmuch new\" to incorporate the new mail
-3. Invoke one or more \"notmuch tag\" commands to classify the mail"
-  :type 'string
+3. Invoke one or more \"notmuch tag\" commands to classify the mail
+
+You can also choose to use \"notmuch new\" through the provided
+`notmuch-poll-script-notmuch-new' function, and have the
+\"notmuch new\" hooks perform the actions above."
+  :type '(choice (const :tag "Not set" nil)
+		 (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
+		 (function :tag "Custom function"
+			   :value notmuch-poll-script-notmuch-new)
+		 (string :tag "Custom script"))
   :group 'notmuch)
 
+(defun notmuch-poll-script-notmuch-new ()
+  "Run \"notmuch new\"."
+  (call-process notmuch-command nil nil nil "new"))
+
 (defun notmuch-poll ()
-  "Run external script to import mail.
+  "Run external script or call a function to import mail.
 
-Invokes `notmuch-poll-script' if it is not set to an empty string."
+Invokes `notmuch-poll-script', which can be a function or the
+name of an external script. Does nothing if `notmuch-poll-script'
+is nil or an empty string."
   (interactive)
-  (if (not (string= notmuch-poll-script ""))
-      (call-process notmuch-poll-script nil nil)))
+  (cond
+   ((stringp notmuch-poll-script)
+    (if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
+	(call-process notmuch-poll-script nil nil)))
+   ((functionp notmuch-poll-script)
+    (funcall notmuch-poll-script))))
 
 (defun notmuch-search-poll-and-refresh-view ()
   "Invoke `notmuch-poll' to import mail, then refresh the current view."
-- 
1.7.5.4

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> Let notmuch-poll-script be a function as well as a string. Make default
> value nil instead of an empty string, but allow "" for backwards
> compatibility. Add a notmuch poll function to call "notmuch new" using the
> configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 

I was just thinking about working on this myself :)

I think a better solution would be to allow running a command with
arguments.  Creating a elisp function just to run a command with some
parameters feels wrong.  This way we would have to add another function
each time we want to add another argument.

Function support for notmuch-poll-script seems like a useful feature on
it's own.

Regards,
  Dmitry

> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  emacs/notmuch.el |   44 ++++++++++++++++++++++++++++++++------------
>  1 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..6c7e800 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,48 @@ same relative position within the new buffer."
>      (notmuch-search query oldest-first target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> -(defcustom notmuch-poll-script ""
> -  "An external script to incorporate new mail into the notmuch database.
> +(defcustom notmuch-poll-script nil
> +  "A script or a function to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> -`notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +This variable can be set to a function or the name of an external
> +script to be invoked by `notmuch-search-poll-and-refresh-view'
> +and `notmuch-hello-poll-and-update' (each have a default
> +keybinding of 'G'). Set to nil to do nothing.
> +
> +The function or script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +You can also choose to use \"notmuch new\" through the provided
> +`notmuch-poll-script-notmuch-new' function, and have the
> +\"notmuch new\" hooks perform the actions above."
> +  :type '(choice (const :tag "Not set" nil)
> +		 (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> +		 (function :tag "Custom function"
> +			   :value notmuch-poll-script-notmuch-new)
> +		 (string :tag "Custom script"))
>    :group 'notmuch)
>  
> +(defun notmuch-poll-script-notmuch-new ()
> +  "Run \"notmuch new\"."
> +  (call-process notmuch-command nil nil nil "new"))
> +
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run external script or call a function to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', which can be a function or the
> +name of an external script. Does nothing if `notmuch-poll-script'
> +is nil or an empty string."
>    (interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -      (call-process notmuch-poll-script nil nil)))
> +  (cond
> +   ((stringp notmuch-poll-script)
> +    (if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
> +	(call-process notmuch-poll-script nil nil)))
> +   ((functionp notmuch-poll-script)
> +    (funcall notmuch-poll-script))))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

Hi Dmitry -

On Mon, 12 Dec 2011 02:00:45 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> > Let notmuch-poll-script be a function as well as a string. Make default
> > value nil instead of an empty string, but allow "" for backwards
> > compatibility. Add a notmuch poll function to call "notmuch new" using the
> > configured notmuch-command.
> > 
> > This allows taking better advantage of the "notmuch new" hooks from emacs
> > without intermediate scripts.
> > 
> 
> I was just thinking about working on this myself :)

:)

> I think a better solution would be to allow running a command with
> arguments.  Creating a elisp function just to run a command with some
> parameters feels wrong.  This way we would have to add another function
> each time we want to add another argument.

One thing to take into account is running "notmuch new" using
notmuch-command, and make that happen with one click in custom.

> Function support for notmuch-poll-script seems like a useful feature on
> it's own.

I just did this quickly to scratch my own itches, so to speak. My elisp
is not that great, so I really wouldn't mind if you wanted to continue
from here and make it your own. It'll be a while before I have the time
to (figure out how to) do this properly anyway. But up to you, really.

BR,
Jani.


> 
> Regards,
>   Dmitry
> 
> > Signed-off-by: Jani Nikula <jani@nikula.org>
> > ---
> >  emacs/notmuch.el |   44 ++++++++++++++++++++++++++++++++------------
> >  1 files changed, 32 insertions(+), 12 deletions(-)
> > 
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 8936149..6c7e800 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -965,28 +965,48 @@ same relative position within the new buffer."
> >      (notmuch-search query oldest-first target-thread target-line continuation)
> >      (goto-char (point-min))))
> >  
> > -(defcustom notmuch-poll-script ""
> > -  "An external script to incorporate new mail into the notmuch database.
> > +(defcustom notmuch-poll-script nil
> > +  "A script or a function to incorporate new mail into the notmuch database.
> >  
> > -If this variable is non empty, then it should name a script to be
> > -invoked by `notmuch-search-poll-and-refresh-view' and
> > -`notmuch-hello-poll-and-update' (each have a default keybinding
> > -of 'G'). The script could do any of the following depending on
> > +This variable can be set to a function or the name of an external
> > +script to be invoked by `notmuch-search-poll-and-refresh-view'
> > +and `notmuch-hello-poll-and-update' (each have a default
> > +keybinding of 'G'). Set to nil to do nothing.
> > +
> > +The function or script could do any of the following depending on
> >  the user's needs:
> >  
> >  1. Invoke a program to transfer mail to the local mail store
> >  2. Invoke \"notmuch new\" to incorporate the new mail
> > -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> > -  :type 'string
> > +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> > +
> > +You can also choose to use \"notmuch new\" through the provided
> > +`notmuch-poll-script-notmuch-new' function, and have the
> > +\"notmuch new\" hooks perform the actions above."
> > +  :type '(choice (const :tag "Not set" nil)
> > +		 (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> > +		 (function :tag "Custom function"
> > +			   :value notmuch-poll-script-notmuch-new)
> > +		 (string :tag "Custom script"))
> >    :group 'notmuch)
> >  
> > +(defun notmuch-poll-script-notmuch-new ()
> > +  "Run \"notmuch new\"."
> > +  (call-process notmuch-command nil nil nil "new"))
> > +
> >  (defun notmuch-poll ()
> > -  "Run external script to import mail.
> > +  "Run external script or call a function to import mail.
> >  
> > -Invokes `notmuch-poll-script' if it is not set to an empty string."
> > +Invokes `notmuch-poll-script', which can be a function or the
> > +name of an external script. Does nothing if `notmuch-poll-script'
> > +is nil or an empty string."
> >    (interactive)
> > -  (if (not (string= notmuch-poll-script ""))
> > -      (call-process notmuch-poll-script nil nil)))
> > +  (cond
> > +   ((stringp notmuch-poll-script)
> > +    (if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
> > +	(call-process notmuch-poll-script nil nil)))
> > +   ((functionp notmuch-poll-script)
> > +    (funcall notmuch-poll-script))))
> >  
> >  (defun notmuch-search-poll-and-refresh-view ()
> >    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> > -- 
> > 1.7.5.4
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 00:19:36 +0200, Jani Nikula <jani@nikula.org> wrote:
> 
> Hi Dmitry -
> 
> On Mon, 12 Dec 2011 02:00:45 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > Let notmuch-poll-script be a function as well as a string. Make default
> > > value nil instead of an empty string, but allow "" for backwards
> > > compatibility. Add a notmuch poll function to call "notmuch new" using the
> > > configured notmuch-command.
> > > 
> > > This allows taking better advantage of the "notmuch new" hooks from emacs
> > > without intermediate scripts.
> > > 
> > 
> > I was just thinking about working on this myself :)
> 
> :)
> 
> > I think a better solution would be to allow running a command with
> > arguments.  Creating a elisp function just to run a command with some
> > parameters feels wrong.  This way we would have to add another function
> > each time we want to add another argument.
> 
> One thing to take into account is running "notmuch new" using
> notmuch-command, and make that happen with one click in custom.
> 

Indeed.  One click in custom should be easy, but using notmuch-command
may not be so.  So now I like your solution :) Though others may think
of something better.

The only comment I have:

+		 (function :tag "Custom function"
+			   :value notmuch-poll-script-notmuch-new)

Should we set :value to notmuch-poll-script-notmuch-new here?  There is
another option for "notmuch new", so perhaps there should be no value
for custom function as for custom script?

Regards,
  Dmitry

> > Function support for notmuch-poll-script seems like a useful feature on
> > it's own.
> 
> I just did this quickly to scratch my own itches, so to speak. My elisp
> is not that great, so I really wouldn't mind if you wanted to continue
> from here and make it your own. It'll be a while before I have the time
> to (figure out how to) do this properly anyway. But up to you, really.
> 
> BR,
> Jani.
> 
> 
> > 
> > Regards,
> >   Dmitry
> > 
> > > Signed-off-by: Jani Nikula <jani@nikula.org>
> > > ---
> > >  emacs/notmuch.el |   44 ++++++++++++++++++++++++++++++++------------
> > >  1 files changed, 32 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > > index 8936149..6c7e800 100644
> > > --- a/emacs/notmuch.el
> > > +++ b/emacs/notmuch.el
> > > @@ -965,28 +965,48 @@ same relative position within the new buffer."
> > >      (notmuch-search query oldest-first target-thread target-line continuation)
> > >      (goto-char (point-min))))
> > >  
> > > -(defcustom notmuch-poll-script ""
> > > -  "An external script to incorporate new mail into the notmuch database.
> > > +(defcustom notmuch-poll-script nil
> > > +  "A script or a function to incorporate new mail into the notmuch database.
> > >  
> > > -If this variable is non empty, then it should name a script to be
> > > -invoked by `notmuch-search-poll-and-refresh-view' and
> > > -`notmuch-hello-poll-and-update' (each have a default keybinding
> > > -of 'G'). The script could do any of the following depending on
> > > +This variable can be set to a function or the name of an external
> > > +script to be invoked by `notmuch-search-poll-and-refresh-view'
> > > +and `notmuch-hello-poll-and-update' (each have a default
> > > +keybinding of 'G'). Set to nil to do nothing.
> > > +
> > > +The function or script could do any of the following depending on
> > >  the user's needs:
> > >  
> > >  1. Invoke a program to transfer mail to the local mail store
> > >  2. Invoke \"notmuch new\" to incorporate the new mail
> > > -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> > > -  :type 'string
> > > +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> > > +
> > > +You can also choose to use \"notmuch new\" through the provided
> > > +`notmuch-poll-script-notmuch-new' function, and have the
> > > +\"notmuch new\" hooks perform the actions above."
> > > +  :type '(choice (const :tag "Not set" nil)
> > > +		 (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> > > +		 (function :tag "Custom function"
> > > +			   :value notmuch-poll-script-notmuch-new)
> > > +		 (string :tag "Custom script"))
> > >    :group 'notmuch)
> > >  
> > > +(defun notmuch-poll-script-notmuch-new ()
> > > +  "Run \"notmuch new\"."
> > > +  (call-process notmuch-command nil nil nil "new"))
> > > +
> > >  (defun notmuch-poll ()
> > > -  "Run external script to import mail.
> > > +  "Run external script or call a function to import mail.
> > >  
> > > -Invokes `notmuch-poll-script' if it is not set to an empty string."
> > > +Invokes `notmuch-poll-script', which can be a function or the
> > > +name of an external script. Does nothing if `notmuch-poll-script'
> > > +is nil or an empty string."
> > >    (interactive)
> > > -  (if (not (string= notmuch-poll-script ""))
> > > -      (call-process notmuch-poll-script nil nil)))
> > > +  (cond
> > > +   ((stringp notmuch-poll-script)
> > > +    (if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
> > > +	(call-process notmuch-poll-script nil nil)))
> > > +   ((functionp notmuch-poll-script)
> > > +    (funcall notmuch-poll-script))))
> > >  
> > >  (defun notmuch-search-poll-and-refresh-view ()
> > >    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> > > -- 
> > > 1.7.5.4
> > > 
> > > _______________________________________________
> > > notmuch mailing list
> > > notmuch@notmuchmail.org
> > > http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

Quoth Dmitry Kurochkin on Dec 12 at  2:00 am:
> Hi Jani.
> 
> On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> > Let notmuch-poll-script be a function as well as a string. Make default
> > value nil instead of an empty string, but allow "" for backwards
> > compatibility. Add a notmuch poll function to call "notmuch new" using the
> > configured notmuch-command.
> > 
> > This allows taking better advantage of the "notmuch new" hooks from emacs
> > without intermediate scripts.
> > 
> 
> I was just thinking about working on this myself :)
> 
> I think a better solution would be to allow running a command with
> arguments.  Creating a elisp function just to run a command with some
> parameters feels wrong.  This way we would have to add another function
> each time we want to add another argument.

This seems a little awkward to me, too, though perhaps it's the best
way.  Other approaches to consider include accepting a list for
notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
string but treating it as a shell command so "notmuch new" would Just
Work.  Personally, I think the latter is the most intuitive, but it
would be worth looking at how other customizable external commands are
done in Emacs.

A function seems powerful, but also like overkill.  Can you give a use
case for a function that wouldn't be more easily solved by one of the
above approaches?

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Dec 12, 2011 12:56 AM, "Austin Clements" <amdragon@mit.edu> wrote:
>
> Quoth Dmitry Kurochkin on Dec 12 at  2:00 am:
> > Hi Jani.
> >
> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > Let notmuch-poll-script be a function as well as a string. Make
default
> > > value nil instead of an empty string, but allow "" for backwards
> > > compatibility. Add a notmuch poll function to call "notmuch new"
using the
> > > configured notmuch-command.
> > >
> > > This allows taking better advantage of the "notmuch new" hooks from
emacs
> > > without intermediate scripts.
> > >
> >
> > I was just thinking about working on this myself :)
> >
> > I think a better solution would be to allow running a command with
> > arguments.  Creating a elisp function just to run a command with some
> > parameters feels wrong.  This way we would have to add another function
> > each time we want to add another argument.
>
> This seems a little awkward to me, too, though perhaps it's the best
> way.  Other approaches to consider include accepting a list for
> notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
> string but treating it as a shell command so "notmuch new" would Just
> Work.  Personally, I think the latter is the most intuitive, but it
> would be worth looking at how other customizable external commands are
> done in Emacs.
>
> A function seems powerful, but also like overkill.  Can you give a use
> case for a function that wouldn't be more easily solved by one of the
> above approaches?

The only reason I had for using a function was running notmuch using
notmuch-command. Any ideas how to do that with the Just Works approach?

J.

[-- Attachment #2: Type: text/html, Size: 2228 bytes --]

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

Quoth Jani Nikula on Dec 12 at  1:10 am:
>    On Dec 12, 2011 12:56 AM, "Austin Clements" <[1]amdragon@mit.edu> wrote:
>    >
>    > Quoth Dmitry Kurochkin on Dec 12 at  2:00 am:
>    > > Hi Jani.
>    > >
>    > > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <[2]jani@nikula.org>
>    wrote:
>    > > > Let notmuch-poll-script be a function as well as a string. Make
>    default
>    > > > value nil instead of an empty string, but allow "" for backwards
>    > > > compatibility. Add a notmuch poll function to call "notmuch new"
>    using the
>    > > > configured notmuch-command.
>    > > >
>    > > > This allows taking better advantage of the "notmuch new" hooks from
>    emacs
>    > > > without intermediate scripts.
>    > > >
>    > >
>    > > I was just thinking about working on this myself :)
>    > >
>    > > I think a better solution would be to allow running a command with
>    > > arguments.  Creating a elisp function just to run a command with some
>    > > parameters feels wrong.  This way we would have to add another
>    function
>    > > each time we want to add another argument.
>    >
>    > This seems a little awkward to me, too, though perhaps it's the best
>    > way.  Other approaches to consider include accepting a list for
>    > notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
>    > string but treating it as a shell command so "notmuch new" would Just
>    > Work.  Personally, I think the latter is the most intuitive, but it
>    > would be worth looking at how other customizable external commands are
>    > done in Emacs.
>    >
>    > A function seems powerful, but also like overkill.  Can you give a use
>    > case for a function that wouldn't be more easily solved by one of the
>    > above approaches?
> 
>    The only reason I had for using a function was running notmuch using
>    notmuch-command. Any ideas how to do that with the Just Works approach?

Oh, I see.  I'd missed that.

So here's another idea, prefaced with a rant.

It's bothered me for a long time that notmuch-emacs didn't just know
by default how to check for new mail.  What MUA doesn't know how to
check for new mail?  Why does a new user of notmuch have to tell it
how to check for new mail?  Of course, this *had* to be configured
before because everyone had their own way of checking for new mail.
Hooks eliminate this unnecessary flexibility and make "notmuch new"
the one true way to check for new mail---as it ought to be---and in
turn make the notmuch-poll-script variable obsolete.

So, what about changing the default "" setting of notmuch-poll-script
from meaning "do nothing and be useless" to meaning "run notmuch new
(using notmuch-command)"?  It will then automatically do the right
thing for new users, while still being backward-compatible and
allowing an escape hatch for bizarre situations.

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Dec 12 at  1:10 am:
> >    On Dec 12, 2011 12:56 AM, "Austin Clements" <[1]amdragon@mit.edu> wrote:
> >    >
> >    > Quoth Dmitry Kurochkin on Dec 12 at  2:00 am:
> >    > > Hi Jani.
> >    > >
> >    > > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <[2]jani@nikula.org>
> >    wrote:
> >    > > > Let notmuch-poll-script be a function as well as a string. Make
> >    default
> >    > > > value nil instead of an empty string, but allow "" for backwards
> >    > > > compatibility. Add a notmuch poll function to call "notmuch new"
> >    using the
> >    > > > configured notmuch-command.
> >    > > >
> >    > > > This allows taking better advantage of the "notmuch new" hooks from
> >    emacs
> >    > > > without intermediate scripts.
> >    > > >
> >    > >
> >    > > I was just thinking about working on this myself :)
> >    > >
> >    > > I think a better solution would be to allow running a command with
> >    > > arguments.  Creating a elisp function just to run a command with some
> >    > > parameters feels wrong.  This way we would have to add another
> >    function
> >    > > each time we want to add another argument.
> >    >
> >    > This seems a little awkward to me, too, though perhaps it's the best
> >    > way.  Other approaches to consider include accepting a list for
> >    > notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
> >    > string but treating it as a shell command so "notmuch new" would Just
> >    > Work.  Personally, I think the latter is the most intuitive, but it
> >    > would be worth looking at how other customizable external commands are
> >    > done in Emacs.
> >    >
> >    > A function seems powerful, but also like overkill.  Can you give a use
> >    > case for a function that wouldn't be more easily solved by one of the
> >    > above approaches?
> > 
> >    The only reason I had for using a function was running notmuch using
> >    notmuch-command. Any ideas how to do that with the Just Works approach?
> 
> Oh, I see.  I'd missed that.
> 
> So here's another idea, prefaced with a rant.
> 
> It's bothered me for a long time that notmuch-emacs didn't just know
> by default how to check for new mail.  What MUA doesn't know how to
> check for new mail?  Why does a new user of notmuch have to tell it
> how to check for new mail?  Of course, this *had* to be configured
> before because everyone had their own way of checking for new mail.
> Hooks eliminate this unnecessary flexibility and make "notmuch new"
> the one true way to check for new mail---as it ought to be---and in
> turn make the notmuch-poll-script variable obsolete.
> 
> So, what about changing the default "" setting of notmuch-poll-script
> from meaning "do nothing and be useless" to meaning "run notmuch new
> (using notmuch-command)"?  It will then automatically do the right
> thing for new users, while still being backward-compatible and
> allowing an escape hatch for bizarre situations.

Fine with me.  AFAIK no one has asked for using custom functions for
notmuch-poll-script, so adding a sane default may be the simplest and
the best option.

Regards,
  Dmitry

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Tomi Ollila]

On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> So here's another idea, prefaced with a rant.
> 
> It's bothered me for a long time that notmuch-emacs didn't just know
> by default how to check for new mail.  What MUA doesn't know how to
> check for new mail?  Why does a new user of notmuch have to tell it
> how to check for new mail?  Of course, this *had* to be configured
> before because everyone had their own way of checking for new mail.
> Hooks eliminate this unnecessary flexibility and make "notmuch new"
> the one true way to check for new mail---as it ought to be---and in
> turn make the notmuch-poll-script variable obsolete.
> 
> So, what about changing the default "" setting of notmuch-poll-script
> from meaning "do nothing and be useless" to meaning "run notmuch new
> (using notmuch-command)"?  It will then automatically do the right
> thing for new users, while still being backward-compatible and
> allowing an escape hatch for bizarre situations.

+1

So, it could work like this:

(defun notmuch-poll ()
  "FIX DOCSTRING"
  (interactive)
  (if (stringp notmuch notmuch-poll-script)
      (if (string= notmuch-poll-script "")
        (call-process notmuch-command nil nil nil "new")
        (call-process notmuch-poll-script))))

I.e. in case notmuch-poll-script == nil, (or not string)
do nothing. In case notmuch-poll-script == "" execute notmuch new
and if notmuch-poll-script is string with content execute that.

users who want other functionality can reimplement notmuch-poll function
after notmuch has been loaded (and manage things themselves when internal
implementation changes ;().


Tomi

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 12:15:44 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > 
> > So here's another idea, prefaced with a rant.
> > 
> > It's bothered me for a long time that notmuch-emacs didn't just know
> > by default how to check for new mail.  What MUA doesn't know how to
> > check for new mail?  Why does a new user of notmuch have to tell it
> > how to check for new mail?  Of course, this *had* to be configured
> > before because everyone had their own way of checking for new mail.
> > Hooks eliminate this unnecessary flexibility and make "notmuch new"
> > the one true way to check for new mail---as it ought to be---and in
> > turn make the notmuch-poll-script variable obsolete.
> > 
> > So, what about changing the default "" setting of notmuch-poll-script
> > from meaning "do nothing and be useless" to meaning "run notmuch new
> > (using notmuch-command)"?  It will then automatically do the right
> > thing for new users, while still being backward-compatible and
> > allowing an escape hatch for bizarre situations.
> 
> +1
> 
> So, it could work like this:
> 
> (defun notmuch-poll ()
>   "FIX DOCSTRING"
>   (interactive)
>   (if (stringp notmuch notmuch-poll-script)
>       (if (string= notmuch-poll-script "")
>         (call-process notmuch-command nil nil nil "new")
>         (call-process notmuch-poll-script))))
> 
> I.e. in case notmuch-poll-script == nil, (or not string)
> do nothing. In case notmuch-poll-script == "" execute notmuch new
> and if notmuch-poll-script is string with content execute that.
> 

I think the following scheme would be slightly better and more
consistent:

* nil - run "notmuch new", the new default
* "" - do nothing
* "script" - run script

Regards,
  Dmitry

> users who want other functionality can reimplement notmuch-poll function
> after notmuch has been loaded (and manage things themselves when internal
> implementation changes ;().
> 
> 
> Tomi
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Tomi Ollila]

On Mon, 12 Dec 2011 14:21:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Mon, 12 Dec 2011 12:15:44 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > > 
> > > So here's another idea, prefaced with a rant.
> > > 
> > > It's bothered me for a long time that notmuch-emacs didn't just know
> > > by default how to check for new mail.  What MUA doesn't know how to
> > > check for new mail?  Why does a new user of notmuch have to tell it
> > > how to check for new mail?  Of course, this *had* to be configured
> > > before because everyone had their own way of checking for new mail.
> > > Hooks eliminate this unnecessary flexibility and make "notmuch new"
> > > the one true way to check for new mail---as it ought to be---and in
> > > turn make the notmuch-poll-script variable obsolete.
> > > 
> > > So, what about changing the default "" setting of notmuch-poll-script
> > > from meaning "do nothing and be useless" to meaning "run notmuch new
> > > (using notmuch-command)"?  It will then automatically do the right
> > > thing for new users, while still being backward-compatible and
> > > allowing an escape hatch for bizarre situations.
> > 
> > +1
> > 
> > So, it could work like this:
> > 
> > (defun notmuch-poll ()
> >   "FIX DOCSTRING"
> >   (interactive)
> >   (if (stringp notmuch notmuch-poll-script)
> >       (if (string= notmuch-poll-script "")
> >         (call-process notmuch-command nil nil nil "new")
> >         (call-process notmuch-poll-script))))
> > 
> > I.e. in case notmuch-poll-script == nil, (or not string)
> > do nothing. In case notmuch-poll-script == "" execute notmuch new
> > and if notmuch-poll-script is string with content execute that.
> > 
> 
> I think the following scheme would be slightly better and more
> consistent:
> 
> * nil - run "notmuch new", the new default
> * "" - do nothing
> * "script" - run script

You're right. This us MUCH better (something irked me when writing
my suggestion but that was for ultimate backward compability); The
default value for notmuch-poll-script could be changed to nil.

> 
> Regards,
>   Dmitry
> 
> > users who want other functionality can reimplement notmuch-poll function
> > after notmuch has been loaded (and manage things themselves when internal
> > implementation changes ;().
> > 
> > 
> > Tomi

Tomi

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

Quoth Dmitry Kurochkin on Dec 12 at  2:21 pm:
> On Mon, 12 Dec 2011 12:15:44 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > > 
> > > So here's another idea, prefaced with a rant.
> > > 
> > > It's bothered me for a long time that notmuch-emacs didn't just know
> > > by default how to check for new mail.  What MUA doesn't know how to
> > > check for new mail?  Why does a new user of notmuch have to tell it
> > > how to check for new mail?  Of course, this *had* to be configured
> > > before because everyone had their own way of checking for new mail.
> > > Hooks eliminate this unnecessary flexibility and make "notmuch new"
> > > the one true way to check for new mail---as it ought to be---and in
> > > turn make the notmuch-poll-script variable obsolete.
> > > 
> > > So, what about changing the default "" setting of notmuch-poll-script
> > > from meaning "do nothing and be useless" to meaning "run notmuch new
> > > (using notmuch-command)"?  It will then automatically do the right
> > > thing for new users, while still being backward-compatible and
> > > allowing an escape hatch for bizarre situations.
> > 
> > +1
> > 
> > So, it could work like this:
> > 
> > (defun notmuch-poll ()
> >   "FIX DOCSTRING"
> >   (interactive)
> >   (if (stringp notmuch notmuch-poll-script)
> >       (if (string= notmuch-poll-script "")
> >         (call-process notmuch-command nil nil nil "new")
> >         (call-process notmuch-poll-script))))
> > 
> > I.e. in case notmuch-poll-script == nil, (or not string)
> > do nothing. In case notmuch-poll-script == "" execute notmuch new
> > and if notmuch-poll-script is string with content execute that.
> > 
> 
> I think the following scheme would be slightly better and more
> consistent:
> 
> * nil - run "notmuch new", the new default
> * "" - do nothing
> * "script" - run script
> 
> Regards,
>   Dmitry

I like it!

[PATCH v2] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

Support nil value for notmuch-poll-script to run "notmuch new" instead of
an external script, and make this the new default. "notmuch new" is run
using the configured notmuch-command.

This allows taking better advantage of the "notmuch new" hooks from emacs
without intermediate scripts.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 emacs/notmuch.el |   34 ++++++++++++++++++++++++----------
 1 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8936149..5a8ab9d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -965,28 +965,42 @@ same relative position within the new buffer."
     (notmuch-search query oldest-first target-thread target-line continuation)
     (goto-char (point-min))))
 
-(defcustom notmuch-poll-script ""
+(defcustom notmuch-poll-script nil
   "An external script to incorporate new mail into the notmuch database.
 
-If this variable is non empty, then it should name a script to be
-invoked by `notmuch-search-poll-and-refresh-view' and
+This variable controls the action invoked by
+`notmuch-search-poll-and-refresh-view' and
 `notmuch-hello-poll-and-update' (each have a default keybinding
-of 'G'). The script could do any of the following depending on
+of 'G') to incorporate new mail into the notmuch database.
+
+If this variable is non empty, then it should name an external
+script to be run. If set to an empty string, no action is
+invoked. Finally, if set to nil (the default), \"notmuch new\" is
+run using the command specified by `notmuch-command'.
+
+The external script could do any of the following depending on
 the user's needs:
 
 1. Invoke a program to transfer mail to the local mail store
 2. Invoke \"notmuch new\" to incorporate the new mail
-3. Invoke one or more \"notmuch tag\" commands to classify the mail"
-  :type 'string
+3. Invoke one or more \"notmuch tag\" commands to classify the mail
+
+Note that the same can be achieved through \"notmuch new\" hooks."
+  :type '(choice (const :tag "Notmuch new" nil)
+		 (const :tag "Disabled" "")
+		 (string :tag "Custom script"))
   :group 'notmuch)
 
 (defun notmuch-poll ()
-  "Run external script to import mail.
+  "Run \"notmuch new\" or an external script to import mail.
 
-Invokes `notmuch-poll-script' if it is not set to an empty string."
+Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
+depending on the value of `notmuch-poll-script'."
   (interactive)
-  (if (not (string= notmuch-poll-script ""))
-      (call-process notmuch-poll-script nil nil)))
+  (if (stringp notmuch-poll-script)
+      (if (not (string= notmuch-poll-script ""))
+	  (call-process notmuch-poll-script nil nil))
+    (call-process notmuch-command nil nil nil "new")))
 
 (defun notmuch-search-poll-and-refresh-view ()
   "Invoke `notmuch-poll' to import mail, then refresh the current view."
-- 
1.7.5.4

Re: [PATCH v2] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Mon, 12 Dec 2011 21:57:28 +0200, Jani Nikula <jani@nikula.org> wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>

Looks good to me.  Few comments regarding documentation below.

> ---
>  emacs/notmuch.el |   34 ++++++++++++++++++++++++----------
>  1 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..5a8ab9d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,42 @@ same relative position within the new buffer."
>      (notmuch-search query oldest-first target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>    "An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If this variable is non empty, then it should name an external

Please s/non empty/non-empty string/ to make it more clear.

> +script to be run. If set to an empty string, no action is
> +invoked. Finally, if set to nil (the default), \"notmuch new\" is
> +run using the command specified by `notmuch-command'.
> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the same can be achieved through \"notmuch new\" hooks."

s/though/using/?

Would be nice to mention that using hooks is the recommended way for
this.

> +  :type '(choice (const :tag "Notmuch new" nil)

Perhaps s/Notmuch/notmuch/ because we are talking about a command here?

Regards,
  Dmitry

> +		 (const :tag "Disabled" "")
> +		 (string :tag "Custom script"))
>    :group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>    (interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -      (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +      (if (not (string= notmuch-poll-script ""))
> +	  (call-process notmuch-poll-script nil nil))
> +    (call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

Looks good to me.

One documentation nitpick below.

Quoth Jani Nikula on Dec 12 at  9:57 pm:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  emacs/notmuch.el |   34 ++++++++++++++++++++++++----------
>  1 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..5a8ab9d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,42 @@ same relative position within the new buffer."
>      (notmuch-search query oldest-first target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>    "An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If this variable is non empty, then it should name an external
> +script to be run. If set to an empty string, no action is
> +invoked. Finally, if set to nil (the default), \"notmuch new\" is
> +run using the command specified by `notmuch-command'.

The default should probably be given first.  Also, "non empty" is a
bit confusing, since nothing has been said about it being a string at
that point.  So, perhaps something like

If set to nil (the default), new mail is processed by invoking
\"notmuch new\".  Otherwise, this should be set to a string that gives
the name of an external script that processes new mail.  If set to the
empty string, no command will be run.

> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the same can be achieved through \"notmuch new\" hooks."
> +  :type '(choice (const :tag "Notmuch new" nil)
> +		 (const :tag "Disabled" "")
> +		 (string :tag "Custom script"))
>    :group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>    (interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -      (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +      (if (not (string= notmuch-poll-script ""))
> +	  (call-process notmuch-poll-script nil nil))
> +    (call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>    "Invoke `notmuch-poll' to import mail, then refresh the current view."

[PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

Support nil value for notmuch-poll-script to run "notmuch new" instead of
an external script, and make this the new default. "notmuch new" is run
using the configured notmuch-command.

This allows taking better advantage of the "notmuch new" hooks from emacs
without intermediate scripts.

Signed-off-by: Jani Nikula <jani@nikula.org>

---

v3: only documentation changes suggested by Austin and Dmitry.
---
 emacs/notmuch.el |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8936149..675a110 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -965,28 +965,43 @@ same relative position within the new buffer."
     (notmuch-search query oldest-first target-thread target-line continuation)
     (goto-char (point-min))))
 
-(defcustom notmuch-poll-script ""
+(defcustom notmuch-poll-script nil
   "An external script to incorporate new mail into the notmuch database.
 
-If this variable is non empty, then it should name a script to be
-invoked by `notmuch-search-poll-and-refresh-view' and
+This variable controls the action invoked by
+`notmuch-search-poll-and-refresh-view' and
 `notmuch-hello-poll-and-update' (each have a default keybinding
-of 'G'). The script could do any of the following depending on
+of 'G') to incorporate new mail into the notmuch database.
+
+If set to nil (the default), new mail is processed by invoking
+\"notmuch new\". Otherwise, this should be set to a string that
+gives the name of an external script that processes new mail. If
+set to the empty string, no command will be run.
+
+The external script could do any of the following depending on
 the user's needs:
 
 1. Invoke a program to transfer mail to the local mail store
 2. Invoke \"notmuch new\" to incorporate the new mail
-3. Invoke one or more \"notmuch tag\" commands to classify the mail"
-  :type 'string
+3. Invoke one or more \"notmuch tag\" commands to classify the mail
+
+Note that the recommended way of achieving the same is using
+\"notmuch new\" hooks."
+  :type '(choice (const :tag "notmuch new" nil)
+		 (const :tag "Disabled" "")
+		 (string :tag "Custom script"))
   :group 'notmuch)
 
 (defun notmuch-poll ()
-  "Run external script to import mail.
+  "Run \"notmuch new\" or an external script to import mail.
 
-Invokes `notmuch-poll-script' if it is not set to an empty string."
+Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
+depending on the value of `notmuch-poll-script'."
   (interactive)
-  (if (not (string= notmuch-poll-script ""))
-      (call-process notmuch-poll-script nil nil)))
+  (if (stringp notmuch-poll-script)
+      (if (not (string= notmuch-poll-script ""))
+	  (call-process notmuch-poll-script nil nil))
+    (call-process notmuch-command nil nil nil "new")))
 
 (defun notmuch-search-poll-and-refresh-view ()
   "Invoke `notmuch-poll' to import mail, then refresh the current view."
-- 
1.7.5.4

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> 
> ---
> 
> v3: only documentation changes suggested by Austin and Dmitry.
> ---
>  emacs/notmuch.el |   35 +++++++++++++++++++++++++----------
>  1 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..675a110 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,43 @@ same relative position within the new buffer."
>      (notmuch-search query oldest-first target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>    "An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If set to nil (the default), new mail is processed by invoking
> +\"notmuch new\". Otherwise, this should be set to a string that
> +gives the name of an external script that processes new mail. If
> +set to the empty string, no command will be run.

I think this should be "an empty string".  But I may be mistaking.

Regards,
  Dmitry

> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the recommended way of achieving the same is using
> +\"notmuch new\" hooks."
> +  :type '(choice (const :tag "notmuch new" nil)
> +		 (const :tag "Disabled" "")
> +		 (string :tag "Custom script"))
>    :group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>    (interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -      (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +      (if (not (string= notmuch-poll-script ""))
> +	  (call-process notmuch-poll-script nil nil))
> +    (call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

LGTM.

Quoth Jani Nikula on Dec 12 at 10:50 pm:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> 
> ---
> 
> v3: only documentation changes suggested by Austin and Dmitry.

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Jani Nikula]

On Tue, 13 Dec 2011 00:53:05 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> > Support nil value for notmuch-poll-script to run "notmuch new" instead of
> > an external script, and make this the new default. "notmuch new" is run
> > using the configured notmuch-command.
> > 
> > This allows taking better advantage of the "notmuch new" hooks from emacs
> > without intermediate scripts.
> > 
> > Signed-off-by: Jani Nikula <jani@nikula.org>
> > 
> > ---
> > 
> > v3: only documentation changes suggested by Austin and Dmitry.
> > ---
> >  emacs/notmuch.el |   35 +++++++++++++++++++++++++----------
> >  1 files changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 8936149..675a110 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -965,28 +965,43 @@ same relative position within the new buffer."
> >      (notmuch-search query oldest-first target-thread target-line continuation)
> >      (goto-char (point-min))))
> >  
> > -(defcustom notmuch-poll-script ""
> > +(defcustom notmuch-poll-script nil
> >    "An external script to incorporate new mail into the notmuch database.
> >  
> > -If this variable is non empty, then it should name a script to be
> > -invoked by `notmuch-search-poll-and-refresh-view' and
> > +This variable controls the action invoked by
> > +`notmuch-search-poll-and-refresh-view' and
> >  `notmuch-hello-poll-and-update' (each have a default keybinding
> > -of 'G'). The script could do any of the following depending on
> > +of 'G') to incorporate new mail into the notmuch database.
> > +
> > +If set to nil (the default), new mail is processed by invoking
> > +\"notmuch new\". Otherwise, this should be set to a string that
> > +gives the name of an external script that processes new mail. If
> > +set to the empty string, no command will be run.
> 
> I think this should be "an empty string".  But I may be mistaking.

Shameless copy paste from a native speaker, who am I to argue? :)
Austin?

Jani.


> 
> Regards,
>   Dmitry
> 
> > +
> > +The external script could do any of the following depending on
> >  the user's needs:
> >  
> >  1. Invoke a program to transfer mail to the local mail store
> >  2. Invoke \"notmuch new\" to incorporate the new mail
> > -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> > -  :type 'string
> > +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> > +
> > +Note that the recommended way of achieving the same is using
> > +\"notmuch new\" hooks."
> > +  :type '(choice (const :tag "notmuch new" nil)
> > +		 (const :tag "Disabled" "")
> > +		 (string :tag "Custom script"))
> >    :group 'notmuch)
> >  
> >  (defun notmuch-poll ()
> > -  "Run external script to import mail.
> > +  "Run \"notmuch new\" or an external script to import mail.
> >  
> > -Invokes `notmuch-poll-script' if it is not set to an empty string."
> > +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> > +depending on the value of `notmuch-poll-script'."
> >    (interactive)
> > -  (if (not (string= notmuch-poll-script ""))
> > -      (call-process notmuch-poll-script nil nil)))
> > +  (if (stringp notmuch-poll-script)
> > +      (if (not (string= notmuch-poll-script ""))
> > +	  (call-process notmuch-poll-script nil nil))
> > +    (call-process notmuch-command nil nil nil "new")))
> >  
> >  (defun notmuch-search-poll-and-refresh-view ()
> >    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> > -- 
> > 1.7.5.4
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Austin Clements]

Quoth Jani Nikula on Dec 12 at 11:13 pm:
> On Tue, 13 Dec 2011 00:53:05 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > +If set to nil (the default), new mail is processed by invoking
> > > +\"notmuch new\". Otherwise, this should be set to a string that
> > > +gives the name of an external script that processes new mail. If
> > > +set to the empty string, no command will be run.
> > 
> > I think this should be "an empty string".  But I may be mistaking.
> 
> Shameless copy paste from a native speaker, who am I to argue? :)
> Austin?

Either way is grammatically correct.  Really, this is a philosophical
question.  Can two empty strings have different identities?  Or is
there only one empty string in the universe?

(eq "" "") => t
(eq "" (string)) => t
(eq "" (make-string 0 ?a)) => t
(eq "" (substring "a" 1)) => t

It would appear Elisp is squarely in the "there is one empty string"
camp, so "the empty string" would be more correct.

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 16:24:51 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Dec 12 at 11:13 pm:
> > On Tue, 13 Dec 2011 00:53:05 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > > +If set to nil (the default), new mail is processed by invoking
> > > > +\"notmuch new\". Otherwise, this should be set to a string that
> > > > +gives the name of an external script that processes new mail. If
> > > > +set to the empty string, no command will be run.
> > > 
> > > I think this should be "an empty string".  But I may be mistaking.
> > 
> > Shameless copy paste from a native speaker, who am I to argue? :)
> > Austin?
> 
> Either way is grammatically correct.  Really, this is a philosophical
> question.  Can two empty strings have different identities?  Or is
> there only one empty string in the universe?
> 
> (eq "" "") => t
> (eq "" (string)) => t
> (eq "" (make-string 0 ?a)) => t
> (eq "" (substring "a" 1)) => t
> 
> It would appear Elisp is squarely in the "there is one empty string"
> camp, so "the empty string" would be more correct.

Fine with me then :)

Regards,
  Dmitry

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[David Bremner]

On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.

pushed.

d

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Pieter Praet]

On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula <jani@nikula.org> wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> 
> ---
> 
> v3: only documentation changes suggested by Austin and Dmitry.
> ---
>  emacs/notmuch.el |   35 +++++++++++++++++++++++++----------
>  1 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..675a110 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,43 @@ same relative position within the new buffer."
>      (notmuch-search query oldest-first target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>    "An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If set to nil (the default), new mail is processed by invoking
> +\"notmuch new\". Otherwise, this should be set to a string that
> +gives the name of an external script that processes new mail. If
> +set to the empty string, no command will be run.
> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the recommended way of achieving the same is using
> +\"notmuch new\" hooks."
> +  :type '(choice (const :tag "notmuch new" nil)
> +		 (const :tag "Disabled" "")
> +		 (string :tag "Custom script"))
>    :group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>    (interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -      (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +      (if (not (string= notmuch-poll-script ""))
> +	  (call-process notmuch-poll-script nil nil))
> +    (call-process notmuch-command nil nil nil "new")))

Since it seems to be bikeshedding-season:

When `(if (not's don't have an `else'-part, use `unless' instead ?

Patch follows.

>  
>  (defun notmuch-search-poll-and-refresh-view ()
>    "Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Nicely complements the new hooks (for which thanks) !


Peace

-- 
Pieter

[PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Pieter Praet]

Less code, same results, without sacrificing readability.

---
 emacs/notmuch-address.el |    6 +++---
 emacs/notmuch-hello.el   |   20 ++++++++++----------
 emacs/notmuch-show.el    |   12 ++++++------
 emacs/notmuch.el         |    8 ++++----
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 8eba7a0..d72b169 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -37,9 +37,9 @@ line."
 (defvar notmuch-address-history nil)
 
 (defun notmuch-address-message-insinuate ()
-  (if (not (memq notmuch-address-message-alist-member message-completion-alist))
-      (setq message-completion-alist
-	    (push notmuch-address-message-alist-member message-completion-alist))))
+  (unless (memq notmuch-address-message-alist-member message-completion-alist)
+    (setq message-completion-alist
+	  (push notmuch-address-message-alist-member message-completion-alist))))
 
 (defun notmuch-address-options (original)
   (process-lines notmuch-address-command original))
diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 333d4c1..dce2020 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -315,7 +315,7 @@ should be. Returns a cons cell `(tags-per-line width)'."
 
     ;; If the last line was not full (and hence did not include a
     ;; carriage return), insert one now.
-    (if (not (eq (% count tags-per-line) 0))
+    (unless (eq (% count tags-per-line) 0)
 	(widget-insert "\n"))
     found-target-pos))
 
@@ -399,7 +399,7 @@ Complete list of currently available key bindings:
 
   ; Jump through a hoop to get this value from the deprecated variable
   ; name (`notmuch-folders') or from the default value.
-  (if (not notmuch-saved-searches)
+  (unless notmuch-saved-searches
     (setq notmuch-saved-searches (notmuch-saved-searches)))
 
   (if no-display
@@ -565,18 +565,18 @@ Complete list of currently available key bindings:
 	  (widget-insert "\n\n")
 	  (let ((start (point)))
 	    (setq found-target-pos (notmuch-hello-insert-tags alltags-alist widest target))
-	    (if (not final-target-pos)
-		(setq final-target-pos found-target-pos))
+	    (unless final-target-pos
+	      (setq final-target-pos found-target-pos))
 	    (indent-rigidly start (point) notmuch-hello-indent)))
 
 	(widget-insert "\n")
 
-	(if (not notmuch-show-all-tags-list)
-	    (widget-create 'push-button
-			   :notify (lambda (widget &rest ignore)
-				     (setq notmuch-show-all-tags-list t)
-				     (notmuch-hello-update))
-			   "Show all tags")))
+	(unless notmuch-show-all-tags-list
+	  (widget-create 'push-button
+			 :notify (lambda (widget &rest ignore)
+				   (setq notmuch-show-all-tags-list t)
+				   (notmuch-hello-update))
+			 "Show all tags")))
 
       (let ((start (point)))
 	(widget-insert "\n\n")
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5502efd..d3543f0 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -659,8 +659,8 @@ current buffer, if possible."
   ;; part, so we make sure that we're down at the end.
   (goto-char (point-max))
   ;; Ensure that the part ends with a carriage return.
-  (if (not (bolp))
-      (insert "\n")))
+  (unless (bolp)
+    (insert "\n")))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
@@ -740,8 +740,8 @@ current buffer, if possible."
     (setq body-start (point-marker))
     (notmuch-show-insert-body msg (plist-get msg :body) depth)
     ;; Ensure that the body ends with a newline.
-    (if (not (bolp))
-	(insert "\n"))
+    (unless (bolp)
+      (insert "\n"))
     (setq body-end (point-marker))
     (setq content-end (point-marker))
 
@@ -882,8 +882,8 @@ buffer."
       (run-hooks 'notmuch-show-hook))
 
     ;; Move straight to the first open message
-    (if (not (notmuch-show-message-visible-p))
-	(notmuch-show-next-open-message))
+    (unless (notmuch-show-message-visible-p)
+      (notmuch-show-next-open-message))
 
     ;; Set the header line to the subject of the first open message.
     (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 1e61775..ba84494 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -636,8 +636,8 @@ This function advances the next thread when finished."
 			(if notmuch-search-process-filter-data
 			    (insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
 			(insert "End of search results.")
-			(if (not (= exit-status 0))
-			    (insert (format " (process returned %d)" exit-status)))
+			(unless (= exit-status 0)
+			  (insert (format " (process returned %d)" exit-status)))
 			(insert "\n")
 			(if (and atbob
 				 (not (string= notmuch-search-target-thread "found")))
@@ -1006,8 +1006,8 @@ Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
 depending on the value of `notmuch-poll-script'."
   (interactive)
   (if (stringp notmuch-poll-script)
-      (if (not (string= notmuch-poll-script ""))
-	  (call-process notmuch-poll-script nil nil))
+      (unless (string= notmuch-poll-script "")
+	(call-process notmuch-poll-script nil nil))
     (call-process notmuch-command nil nil nil "new")))
 
 (defun notmuch-search-poll-and-refresh-view ()
-- 
1.7.8.1

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Dmitry Kurochkin]

On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 

+1

Regards,
  Dmitry

> ---
>  emacs/notmuch-address.el |    6 +++---
>  emacs/notmuch-hello.el   |   20 ++++++++++----------
>  emacs/notmuch-show.el    |   12 ++++++------
>  emacs/notmuch.el         |    8 ++++----
>  4 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
> index 8eba7a0..d72b169 100644
> --- a/emacs/notmuch-address.el
> +++ b/emacs/notmuch-address.el
> @@ -37,9 +37,9 @@ line."
>  (defvar notmuch-address-history nil)
>  
>  (defun notmuch-address-message-insinuate ()
> -  (if (not (memq notmuch-address-message-alist-member message-completion-alist))
> -      (setq message-completion-alist
> -	    (push notmuch-address-message-alist-member message-completion-alist))))
> +  (unless (memq notmuch-address-message-alist-member message-completion-alist)
> +    (setq message-completion-alist
> +	  (push notmuch-address-message-alist-member message-completion-alist))))
>  
>  (defun notmuch-address-options (original)
>    (process-lines notmuch-address-command original))
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 333d4c1..dce2020 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -315,7 +315,7 @@ should be. Returns a cons cell `(tags-per-line width)'."
>  
>      ;; If the last line was not full (and hence did not include a
>      ;; carriage return), insert one now.
> -    (if (not (eq (% count tags-per-line) 0))
> +    (unless (eq (% count tags-per-line) 0)
>  	(widget-insert "\n"))
>      found-target-pos))
>  
> @@ -399,7 +399,7 @@ Complete list of currently available key bindings:
>  
>    ; Jump through a hoop to get this value from the deprecated variable
>    ; name (`notmuch-folders') or from the default value.
> -  (if (not notmuch-saved-searches)
> +  (unless notmuch-saved-searches
>      (setq notmuch-saved-searches (notmuch-saved-searches)))
>  
>    (if no-display
> @@ -565,18 +565,18 @@ Complete list of currently available key bindings:
>  	  (widget-insert "\n\n")
>  	  (let ((start (point)))
>  	    (setq found-target-pos (notmuch-hello-insert-tags alltags-alist widest target))
> -	    (if (not final-target-pos)
> -		(setq final-target-pos found-target-pos))
> +	    (unless final-target-pos
> +	      (setq final-target-pos found-target-pos))
>  	    (indent-rigidly start (point) notmuch-hello-indent)))
>  
>  	(widget-insert "\n")
>  
> -	(if (not notmuch-show-all-tags-list)
> -	    (widget-create 'push-button
> -			   :notify (lambda (widget &rest ignore)
> -				     (setq notmuch-show-all-tags-list t)
> -				     (notmuch-hello-update))
> -			   "Show all tags")))
> +	(unless notmuch-show-all-tags-list
> +	  (widget-create 'push-button
> +			 :notify (lambda (widget &rest ignore)
> +				   (setq notmuch-show-all-tags-list t)
> +				   (notmuch-hello-update))
> +			 "Show all tags")))
>  
>        (let ((start (point)))
>  	(widget-insert "\n\n")
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5502efd..d3543f0 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -659,8 +659,8 @@ current buffer, if possible."
>    ;; part, so we make sure that we're down at the end.
>    (goto-char (point-max))
>    ;; Ensure that the part ends with a carriage return.
> -  (if (not (bolp))
> -      (insert "\n")))
> +  (unless (bolp)
> +    (insert "\n")))
>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> @@ -740,8 +740,8 @@ current buffer, if possible."
>      (setq body-start (point-marker))
>      (notmuch-show-insert-body msg (plist-get msg :body) depth)
>      ;; Ensure that the body ends with a newline.
> -    (if (not (bolp))
> -	(insert "\n"))
> +    (unless (bolp)
> +      (insert "\n"))
>      (setq body-end (point-marker))
>      (setq content-end (point-marker))
>  
> @@ -882,8 +882,8 @@ buffer."
>        (run-hooks 'notmuch-show-hook))
>  
>      ;; Move straight to the first open message
> -    (if (not (notmuch-show-message-visible-p))
> -	(notmuch-show-next-open-message))
> +    (unless (notmuch-show-message-visible-p)
> +      (notmuch-show-next-open-message))
>  
>      ;; Set the header line to the subject of the first open message.
>      (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 1e61775..ba84494 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -636,8 +636,8 @@ This function advances the next thread when finished."
>  			(if notmuch-search-process-filter-data
>  			    (insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
>  			(insert "End of search results.")
> -			(if (not (= exit-status 0))
> -			    (insert (format " (process returned %d)" exit-status)))
> +			(unless (= exit-status 0)
> +			  (insert (format " (process returned %d)" exit-status)))
>  			(insert "\n")
>  			(if (and atbob
>  				 (not (string= notmuch-search-target-thread "found")))
> @@ -1006,8 +1006,8 @@ Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
>  depending on the value of `notmuch-poll-script'."
>    (interactive)
>    (if (stringp notmuch-poll-script)
> -      (if (not (string= notmuch-poll-script ""))
> -	  (call-process notmuch-poll-script nil nil))
> +      (unless (string= notmuch-poll-script "")
> +	(call-process notmuch-poll-script nil nil))
>      (call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
> -- 
> 1.7.8.1
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Xavier Maillard]

On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.

+1

/Xavier

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.

+1, but why not replace non-branching `if' with `when' as well?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Tomi Ollila]

On Fri, 13 Jan 2012 13:42:55 +0100, Xavier Maillard <xavier@maillard.im> wrote:
> 
> On Fri, 13 Jan 2012 08:23:55 +0000, David Edmondson <dme@dme.org> wrote:
> > On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > Less code, same results, without sacrificing readability.
> > 
> > +1, but why not replace non-branching `if' with `when' as well?
> 
> I tend to use WHEN for case I need to execute multiple
> statements. For the rest, IF is ok.

Same here, (when ...) is (if (progn ...)) for me.

> 
> /Xavier

Tomi

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Xavier Maillard]

On Fri, 13 Jan 2012 08:23:55 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> 
> +1, but why not replace non-branching `if' with `when' as well?

I tend to use WHEN for case I need to execute multiple
statements. For the rest, IF is ok.

/Xavier

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On Fri, 13 Jan 2012 13:42:55 +0100, Xavier Maillard <xavier@maillard.im> wrote:
> On Fri, 13 Jan 2012 08:23:55 +0000, David Edmondson <dme@dme.org> wrote:
> > On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > Less code, same results, without sacrificing readability.
> > 
> > +1, but why not replace non-branching `if' with `when' as well?
> 
> I tend to use WHEN for case I need to execute multiple
> statements. For the rest, IF is ok.

Understood.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.

Does this change correctly re-indent the line following the if/unless?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Pieter Praet]

On Fri, 13 Jan 2012 08:23:55 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> 
> +1, but why not replace non-branching `if' with `when' as well?

I was planning to do that when the `unless' patch was accepted,
but after reading Xavier and Tomi's replies, I've changed my mind.

Looking at "subr.el", it's actually more efficient to use `if'
(implemented in C) instead of `when' (a macro, which essentially
runs "if conf (progn ...)".

The amount of non-branching "(if COND (progn ..."  statements is very
limited though, so if inclined to "fix" them nonetheless, a patch
follows (relative to the previous one!).


Peace

-- 
Pieter

[PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

Less code, same results, without sacrificing readability.

---
 emacs/notmuch-show.el |   20 +++++++++-----------
 emacs/notmuch-wash.el |   47 +++++++++++++++++++++++------------------------
 emacs/notmuch.el      |   28 +++++++++++++---------------
 3 files changed, 45 insertions(+), 50 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 0cbf354..1e190ae 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1226,11 +1226,10 @@ any effects from previous calls to
       ;; If a small number of lines from the previous message are
       ;; visible, realign so that the top of the current message is at
       ;; the top of the screen.
-      (if (<= (count-screen-lines (window-start) start-of-message)
-	      next-screen-context-lines)
-	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
+      (when (<= (count-screen-lines (window-start) start-of-message)
+		next-screen-context-lines)
+	(goto-char (notmuch-show-message-top))
+	(notmuch-show-message-adjust))
       ;; Move to the top left of the window.
       (goto-char (window-start)))
      (t
@@ -1423,12 +1422,11 @@ argument, hide all of the messages."
   ;; Move to the next item in the search results, if any.
   (let ((parent-buffer notmuch-show-parent-buffer))
     (notmuch-kill-this-buffer)
-    (if parent-buffer
-	(progn
-	  (switch-to-buffer parent-buffer)
-	  (forward-line)
-	  (if show-next
-	      (notmuch-search-show-thread))))))
+    (when parent-buffer
+      (switch-to-buffer parent-buffer)
+      (forward-line)
+      (if show-next
+	  (notmuch-search-show-thread)))))
 
 (defun notmuch-show-archive-thread ()
   "Archive each message in thread, then show next thread from search.
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 5c1e830..67143e5 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -336,30 +336,29 @@ patch and then guesses the extent of the patch, there is scope
 for error."
 
   (goto-char (point-min))
-  (if (re-search-forward diff-file-header-re nil t)
-      (progn
-	(beginning-of-line -1)
-	(let ((patch-start (point))
-	      (patch-end (point-max))
-	      part)
-	  (goto-char patch-start)
-	  (if (or
-	       ;; Patch ends with signature.
-	       (re-search-forward notmuch-wash-signature-regexp nil t)
-	       ;; Patch ends with bugtraq comment.
-	       (re-search-forward "^\\*\\*\\* " nil t))
-	      (setq patch-end (match-beginning 0)))
-	  (save-restriction
-	    (narrow-to-region patch-start patch-end)
-	    (setq part (plist-put part :content-type "inline-patch-fake-part"))
-	    (setq part (plist-put part :content (buffer-string)))
-	    (setq part (plist-put part :id -1))
-	    (setq part (plist-put part :filename
-				  (notmuch-wash-subject-to-patch-filename
-				   (plist-get
-				    (plist-get msg :headers) :Subject))))
-	    (delete-region (point-min) (point-max))
-	    (notmuch-show-insert-bodypart nil part depth))))))
+  (when (re-search-forward diff-file-header-re nil t)
+    (beginning-of-line -1)
+    (let ((patch-start (point))
+	  (patch-end (point-max))
+	  part)
+      (goto-char patch-start)
+      (if (or
+	   ;; Patch ends with signature.
+	   (re-search-forward notmuch-wash-signature-regexp nil t)
+	   ;; Patch ends with bugtraq comment.
+	   (re-search-forward "^\\*\\*\\* " nil t))
+	  (setq patch-end (match-beginning 0)))
+      (save-restriction
+	(narrow-to-region patch-start patch-end)
+	(setq part (plist-put part :content-type "inline-patch-fake-part"))
+	(setq part (plist-put part :content (buffer-string)))
+	(setq part (plist-put part :id -1))
+	(setq part (plist-put part :filename
+			      (notmuch-wash-subject-to-patch-filename
+			       (plist-get
+				(plist-get msg :headers) :Subject))))
+	(delete-region (point-min) (point-max))
+	(notmuch-show-insert-bodypart nil part depth)))))
 
 ;;
 
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ba84494..0d37a74 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -631,17 +631,16 @@ This function advances the next thread when finished."
 		  (goto-char (point-max))
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
-		  (if (eq status 'exit)
-		      (progn
-			(if notmuch-search-process-filter-data
-			    (insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
-			(insert "End of search results.")
-			(unless (= exit-status 0)
-			  (insert (format " (process returned %d)" exit-status)))
-			(insert "\n")
-			(if (and atbob
-				 (not (string= notmuch-search-target-thread "found")))
-			    (set 'never-found-target-thread t))))))
+		  (when (eq status 'exit)
+		    (if notmuch-search-process-filter-data
+			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
+		    (insert "End of search results.")
+		    (unless (= exit-status 0)
+		      (insert (format " (process returned %d)" exit-status)))
+		    (insert "\n")
+		    (if (and atbob
+			     (not (string= notmuch-search-target-thread "found")))
+			(set 'never-found-target-thread t)))))
 	      (when (and never-found-target-thread
 		       notmuch-search-target-line)
 		  (goto-char (point-min))
@@ -818,10 +817,9 @@ non-authors is found, assume that all of the authors match."
 			(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
 			(put-text-property beg (point) 'notmuch-search-authors authors)
 			(put-text-property beg (point) 'notmuch-search-subject subject)
-			(if (string= thread-id notmuch-search-target-thread)
-			    (progn
-			      (set 'found-target beg)
-			      (set 'notmuch-search-target-thread "found"))))
+			(when (string= thread-id notmuch-search-target-thread)
+			  (set 'found-target beg)
+			  (set 'notmuch-search-target-thread "found")))
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.8.1

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Pieter Praet]

On Fri, 13 Jan 2012 16:06:17 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> 
> Does this change correctly re-indent the line following the if/unless?

It does...

Or so I thought... I appear to have forgotten to correct the indentation
@ `notmuch-hello-insert-tags'.


Unfortunately git doesn't provide a way to diff the whitespace itself;
  (eg. "git diff --word-diff=color --word-diff-regex='[ \t]*'")

Setting "color.diff.whitespace" (and perhaps expanding the scope of
"core.whitespace") only colorizes *erroneous* whitespace...


Does this really warrant a v2, or might we simply leave it as yet
another victim for Tomi's uncrustify-for-elisp [1] ?


Peace

-- 
Pieter

[1] id:"yf639bkexs3.fsf@taco2.nixu.fi"

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Pieter Praet]

On Sat, 14 Jan 2012 10:14:57 +0100, Pieter Praet <pieter@praet.org> wrote:
> On Fri, 13 Jan 2012 08:23:55 +0000, David Edmondson <dme@dme.org> wrote:
> > On Thu, 12 Jan 2012 18:33:06 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > Less code, same results, without sacrificing readability.
> > 
> > +1, but why not replace non-branching `if' with `when' as well?
> 
> I was planning to do that when the `unless' patch was accepted,
                    ^^
                   submit

> but after reading Xavier and Tomi's replies, I've changed my mind.
                                               ^^^^^^^^^^^^^^^^^^^^
                                               I hadn't, though...

> 
> Looking at "subr.el", it's actually more efficient to use `if'
> (implemented in C) instead of `when' (a macro, which essentially
> runs "if conf (progn ...)".
> 
> The amount of non-branching "(if COND (progn ..."  statements is very
> limited though, so if inclined to "fix" them nonetheless, a patch
> follows (relative to the previous one!).
> 
> 
> Peace
> 
> -- 
> Pieter

Typsos FTW!

-- 
Pieter

Re: [PATCH] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

On Sat, 14 Jan 2012 10:18:46 +0100, Pieter Praet <pieter@praet.org> wrote:
> Does this really warrant a v2, or might we simply leave it as yet
> another victim for Tomi's uncrustify-for-elisp [1] ?

Pushing incorrectly indented code should be banned, in my opinion[1]. I'd
be tempted to have some pre-commit hooks to detect it.

Footnotes: 
[1]  Though I've done it myself.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v2] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Pieter Praet]

Less code, same results, without sacrificing readability.

---

v2: Fixed indentation oversight @ `notmuch-hello-insert-tags'.

 emacs/notmuch-address.el |    6 +++---
 emacs/notmuch-hello.el   |   22 +++++++++++-----------
 emacs/notmuch-show.el    |   12 ++++++------
 emacs/notmuch.el         |    8 ++++----
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 8eba7a0..d72b169 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -37,9 +37,9 @@ line."
 (defvar notmuch-address-history nil)
 
 (defun notmuch-address-message-insinuate ()
-  (if (not (memq notmuch-address-message-alist-member message-completion-alist))
-      (setq message-completion-alist
-	    (push notmuch-address-message-alist-member message-completion-alist))))
+  (unless (memq notmuch-address-message-alist-member message-completion-alist)
+    (setq message-completion-alist
+	  (push notmuch-address-message-alist-member message-completion-alist))))
 
 (defun notmuch-address-options (original)
   (process-lines notmuch-address-command original))
diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 02017ce..8aa2ad5 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -317,8 +317,8 @@ should be. Returns a cons cell `(tags-per-line width)'."
 
     ;; If the last line was not full (and hence did not include a
     ;; carriage return), insert one now.
-    (if (not (eq (% count tags-per-line) 0))
-	(widget-insert "\n"))
+    (unless (eq (% count tags-per-line) 0)
+      (widget-insert "\n"))
     found-target-pos))
 
 (defun notmuch-hello-goto-search ()
@@ -401,7 +401,7 @@ Complete list of currently available key bindings:
 
   ; Jump through a hoop to get this value from the deprecated variable
   ; name (`notmuch-folders') or from the default value.
-  (if (not notmuch-saved-searches)
+  (unless notmuch-saved-searches
     (setq notmuch-saved-searches (notmuch-saved-searches)))
 
   (if no-display
@@ -567,18 +567,18 @@ Complete list of currently available key bindings:
 	  (widget-insert "\n\n")
 	  (let ((start (point)))
 	    (setq found-target-pos (notmuch-hello-insert-tags alltags-alist widest target))
-	    (if (not final-target-pos)
-		(setq final-target-pos found-target-pos))
+	    (unless final-target-pos
+	      (setq final-target-pos found-target-pos))
 	    (indent-rigidly start (point) notmuch-hello-indent)))
 
 	(widget-insert "\n")
 
-	(if (not notmuch-show-all-tags-list)
-	    (widget-create 'push-button
-			   :notify (lambda (widget &rest ignore)
-				     (setq notmuch-show-all-tags-list t)
-				     (notmuch-hello-update))
-			   "Show all tags")))
+	(unless notmuch-show-all-tags-list
+	  (widget-create 'push-button
+			 :notify (lambda (widget &rest ignore)
+				   (setq notmuch-show-all-tags-list t)
+				   (notmuch-hello-update))
+			 "Show all tags")))
 
       (let ((start (point)))
 	(widget-insert "\n\n")
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1a250a3..72bf888 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -656,8 +656,8 @@ current buffer, if possible."
   ;; part, so we make sure that we're down at the end.
   (goto-char (point-max))
   ;; Ensure that the part ends with a carriage return.
-  (if (not (bolp))
-      (insert "\n")))
+  (unless (bolp)
+    (insert "\n")))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
@@ -737,8 +737,8 @@ current buffer, if possible."
     (setq body-start (point-marker))
     (notmuch-show-insert-body msg (plist-get msg :body) depth)
     ;; Ensure that the body ends with a newline.
-    (if (not (bolp))
-	(insert "\n"))
+    (unless (bolp)
+      (insert "\n"))
     (setq body-end (point-marker))
     (setq content-end (point-marker))
 
@@ -879,8 +879,8 @@ buffer."
       (run-hooks 'notmuch-show-hook))
 
     ;; Move straight to the first open message
-    (if (not (notmuch-show-message-visible-p))
-	(notmuch-show-next-open-message))
+    (unless (notmuch-show-message-visible-p)
+      (notmuch-show-next-open-message))
 
     ;; Set the header line to the subject of the first open message.
     (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ef4dcc7..c5fcc80 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -643,8 +643,8 @@ This function advances the next thread when finished."
 			(if notmuch-search-process-filter-data
 			    (insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
 			(insert "End of search results.")
-			(if (not (= exit-status 0))
-			    (insert (format " (process returned %d)" exit-status)))
+			(unless (= exit-status 0)
+			  (insert (format " (process returned %d)" exit-status)))
 			(insert "\n")
 			(if (and atbob
 				 (not (string= notmuch-search-target-thread "found")))
@@ -1013,8 +1013,8 @@ Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
 depending on the value of `notmuch-poll-script'."
   (interactive)
   (if (stringp notmuch-poll-script)
-      (if (not (string= notmuch-poll-script ""))
-	  (call-process notmuch-poll-script nil nil))
+      (unless (string= notmuch-poll-script "")
+	(call-process notmuch-poll-script nil nil))
     (call-process notmuch-command nil nil nil "new")))
 
 (defun notmuch-search-poll-and-refresh-view ()
-- 
1.7.8.1

Re: [PATCH v2] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 140 bytes --]

On Mon, 16 Jan 2012 11:56:40 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.

+1.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v2] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[Tomi Ollila]

On Mon, 16 Jan 2012 11:56:40 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 
> ---

+1

Tomi

Re: [PATCH v2] emacs: globally replace non-branching "(if (not ..." with "(unless ..."

[David Bremner]

On Mon, 16 Jan 2012 11:56:40 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 

pushed

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Bremner]

On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 

This looks OK, although the re-indenting makes these kind of changes
painful to review (not that I'm suggesting we should re-indent, just
some random complaining).

d

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Jan 28, 2012 2:41 PM, "David Bremner" <david@tethera.net> wrote:
>
> On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> >
>
> This looks OK, although the re-indenting makes these kind of changes
> painful to review (not that I'm suggesting we should re-indent, just
> some random complaining).

Sometimes someone (Dmitry?) sent patches that separated a small functional
change, and the big non-functional indentation change it caused,
separately. Would you prefer (or tolerate ;) that style?

>
> d
>
>

[-- Attachment #2: Type: text/html, Size: 816 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Bremner]

On Sat, 28 Jan 2012 14:55:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> On Jan 28, 2012 2:41 PM, "David Bremner" <david@tethera.net> wrote:
> 
> Sometimes someone (Dmitry?) sent patches that separated a small functional
> change, and the big non-functional indentation change it caused,
> separately. Would you prefer (or tolerate ;) that style?

Hmm, that might be nicer, I'm not 100% sure.

I wouldn't say it's mandatory for a patch like this (and I'd say other
peoples views on what's easy to review are at least as important as mine
here).

d

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

On Sat, 28 Jan 2012 08:41:36 -0400, David Bremner <david@tethera.net> wrote:
> On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> > 
> 
> This looks OK, although the re-indenting makes these kind of changes
> painful to review (not that I'm suggesting we should re-indent, just
> some random complaining).
> 

You can use `diff-refine-hunk' to see what the actual changes are.

Try this:

  #+begin_src emacs-lisp
    (global-set-key (kbd "C-c /")
                    (lambda()
                      "Refine display of unified diff hunks"
                      (interactive)
                      (save-excursion
                        (goto-char (point-min))
                        (while (re-search-forward
                                diff-hunk-header-re-unified
                                nil t)
                          (diff-refine-hunk)))))
  #+end_src

Work pretty much *anywhere*.

Note: it does NOT work in `notmuch-show-mode' (not even with
`notmuch-wash-convert-inline-patch-to-part' disabled), but this is
easily solved by first running `notmuch-show-view-raw-message' ("V")
and `diff-mode' (to fontify the buffer)...

> d
> 
> 


Peace

-- 
Pieter

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Tomi Ollila]

On Mon, 30 Jan 2012 08:03:59 +0100, Pieter Praet <pieter@praet.org> wrote:
> On Sat, 28 Jan 2012 08:41:36 -0400, David Bremner <david@tethera.net> wrote:
> > On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > Less code, same results, without sacrificing readability.
> > > 
> > 
> > This looks OK, although the re-indenting makes these kind of changes
> > painful to review (not that I'm suggesting we should re-indent, just
> > some random complaining).
> > 
> 
> You can use `diff-refine-hunk' to see what the actual changes are.
> 
> Try this:
> 
>   #+begin_src emacs-lisp
>     (global-set-key (kbd "C-c /")
>                     (lambda()
>                       "Refine display of unified diff hunks"
>                       (interactive)
>                       (save-excursion
>                         (goto-char (point-min))
>                         (while (re-search-forward
>                                 diff-hunk-header-re-unified
>                                 nil t)
>                           (diff-refine-hunk)))))
>   #+end_src
> 
> Work pretty much *anywhere*.
> 
> Note: it does NOT work in `notmuch-show-mode' (not even with
> `notmuch-wash-convert-inline-patch-to-part' disabled), but this is
> easily solved by first running `notmuch-show-view-raw-message' ("V")
> and `diff-mode' (to fontify the buffer)...

What we need is 'notmuch-devel' minor mode which can do this, dme's
notmuch patching tool, nmbug tag changes and everything...

> 
> > d
> 
> 
> Peace
> 
> -- 
> Pieter

Tomi

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Sat, 28 Jan 2012 13:14:45 -0400, David Bremner <david@tethera.net> wrote:
> On Sat, 28 Jan 2012 14:55:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> > On Jan 28, 2012 2:41 PM, "David Bremner" <david@tethera.net> wrote:
> > 
> > Sometimes someone (Dmitry?) sent patches that separated a small functional
> > change, and the big non-functional indentation change it caused,
> > separately. Would you prefer (or tolerate ;) that style?
> 
> Hmm, that might be nicer, I'm not 100% sure.
> 
> I wouldn't say it's mandatory for a patch like this (and I'd say other
> peoples views on what's easy to review are at least as important as mine
> here).

Each patch should be valid in the repository without any following
patches (preceding are obviously okay). Incorrect indentation would
disqualify a patch from being 'valid', so it shouldn't be accepted.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Bremner]

On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 
> ---

Sorry, it conflicts with Jamie's patches that I just pushed.

d

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

On Mon, 30 Jan 2012 09:23:40 +0000, David Edmondson <dme@dme.org> wrote:
> On Sat, 28 Jan 2012 13:14:45 -0400, David Bremner <david@tethera.net> wrote:
> > On Sat, 28 Jan 2012 14:55:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > On Jan 28, 2012 2:41 PM, "David Bremner" <david@tethera.net> wrote:
> > > 
> > > Sometimes someone (Dmitry?) sent patches that separated a small functional
> > > change, and the big non-functional indentation change it caused,
> > > separately. Would you prefer (or tolerate ;) that style?
> > 
> > Hmm, that might be nicer, I'm not 100% sure.
> > 
> > I wouldn't say it's mandatory for a patch like this (and I'd say other
> > peoples views on what's easy to review are at least as important as mine
> > here).
> 
> Each patch should be valid in the repository without any following
> patches (preceding are obviously okay). Incorrect indentation would
> disqualify a patch from being 'valid', so it shouldn't be accepted.

+1.

Indentation corrections should always be part of the same patch as
the change(s) that invalidated the indentation in the first place.

Spotting the *actual* (non-indentation) changes is a non-issue
when using `diff-refine-hunk' (or a wrapper thereof [1]).


> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace

-- 
Pieter

[1] id:"8739ax7jts.fsf@praet.org"

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

On Mon, 30 Jan 2012 09:59:32 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Mon, 30 Jan 2012 08:03:59 +0100, Pieter Praet <pieter@praet.org> wrote:
> > On Sat, 28 Jan 2012 08:41:36 -0400, David Bremner <david@tethera.net> wrote:
> > > On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> > > > Less code, same results, without sacrificing readability.
> > > > 
> > > 
> > > This looks OK, although the re-indenting makes these kind of changes
> > > painful to review (not that I'm suggesting we should re-indent, just
> > > some random complaining).
> > > 
> > 
> > You can use `diff-refine-hunk' to see what the actual changes are.
> > 
> > Try this:
> > 
> >   #+begin_src emacs-lisp
> >     (global-set-key (kbd "C-c /")
> >                     (lambda()
> >                       "Refine display of unified diff hunks"
> >                       (interactive)
> >                       (save-excursion
> >                         (goto-char (point-min))
> >                         (while (re-search-forward
> >                                 diff-hunk-header-re-unified
> >                                 nil t)
> >                           (diff-refine-hunk)))))
> >   #+end_src
> > 
> > Work pretty much *anywhere*.
> > 
> > Note: it does NOT work in `notmuch-show-mode' (not even with
> > `notmuch-wash-convert-inline-patch-to-part' disabled), but this is
> > easily solved by first running `notmuch-show-view-raw-message' ("V")
> > and `diff-mode' (to fontify the buffer)...
> 
> What we need is 'notmuch-devel' minor mode which can do this, dme's
> notmuch patching tool, nmbug tag changes and everything...
> 

Full ACK!


> > 
> > > d
> > 
> > 
> > Peace
> > 
> > -- 
> > Pieter
> 
> Tomi


Peace

-- 
Pieter

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

On Mon, 30 Jan 2012 23:34:12 -0400, David Bremner <david@tethera.net> wrote:
> On Sat, 14 Jan 2012 10:17:18 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Less code, same results, without sacrificing readability.
> > 
> > ---
> 
> Sorry, it conflicts with Jamie's patches that I just pushed.
> 

Please don't apologize for doing the right thing :)

Rebased patch follows.


> d


Peace

-- 
Pieter

[PATCH v2] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Pieter Praet]

Less code, same results, without sacrificing readability.

---

Rebased to current master.

 emacs/notmuch-show.el |    9 ++++-----
 emacs/notmuch-wash.el |   47 +++++++++++++++++++++++------------------------
 emacs/notmuch.el      |   28 +++++++++++++---------------
 3 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index de9421e..0a945ea 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1364,11 +1364,10 @@ any effects from previous calls to
       ;; If a small number of lines from the previous message are
       ;; visible, realign so that the top of the current message is at
       ;; the top of the screen.
-      (if (<= (count-screen-lines (window-start) start-of-message)
-	      next-screen-context-lines)
-	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
+      (when (<= (count-screen-lines (window-start) start-of-message)
+		next-screen-context-lines)
+	(goto-char (notmuch-show-message-top))
+	(notmuch-show-message-adjust))
       ;; Move to the top left of the window.
       (goto-char (window-start)))
      (t
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 5c1e830..67143e5 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -336,30 +336,29 @@ patch and then guesses the extent of the patch, there is scope
 for error."
 
   (goto-char (point-min))
-  (if (re-search-forward diff-file-header-re nil t)
-      (progn
-	(beginning-of-line -1)
-	(let ((patch-start (point))
-	      (patch-end (point-max))
-	      part)
-	  (goto-char patch-start)
-	  (if (or
-	       ;; Patch ends with signature.
-	       (re-search-forward notmuch-wash-signature-regexp nil t)
-	       ;; Patch ends with bugtraq comment.
-	       (re-search-forward "^\\*\\*\\* " nil t))
-	      (setq patch-end (match-beginning 0)))
-	  (save-restriction
-	    (narrow-to-region patch-start patch-end)
-	    (setq part (plist-put part :content-type "inline-patch-fake-part"))
-	    (setq part (plist-put part :content (buffer-string)))
-	    (setq part (plist-put part :id -1))
-	    (setq part (plist-put part :filename
-				  (notmuch-wash-subject-to-patch-filename
-				   (plist-get
-				    (plist-get msg :headers) :Subject))))
-	    (delete-region (point-min) (point-max))
-	    (notmuch-show-insert-bodypart nil part depth))))))
+  (when (re-search-forward diff-file-header-re nil t)
+    (beginning-of-line -1)
+    (let ((patch-start (point))
+	  (patch-end (point-max))
+	  part)
+      (goto-char patch-start)
+      (if (or
+	   ;; Patch ends with signature.
+	   (re-search-forward notmuch-wash-signature-regexp nil t)
+	   ;; Patch ends with bugtraq comment.
+	   (re-search-forward "^\\*\\*\\* " nil t))
+	  (setq patch-end (match-beginning 0)))
+      (save-restriction
+	(narrow-to-region patch-start patch-end)
+	(setq part (plist-put part :content-type "inline-patch-fake-part"))
+	(setq part (plist-put part :content (buffer-string)))
+	(setq part (plist-put part :id -1))
+	(setq part (plist-put part :filename
+			      (notmuch-wash-subject-to-patch-filename
+			       (plist-get
+				(plist-get msg :headers) :Subject))))
+	(delete-region (point-min) (point-max))
+	(notmuch-show-insert-bodypart nil part depth)))))
 
 ;;
 
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 72f78ed..5fa239a 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -673,17 +673,16 @@ This function advances the next thread when finished."
 		  (goto-char (point-max))
 		  (if (eq status 'signal)
 		      (insert "Incomplete search results (search process was killed).\n"))
-		  (if (eq status 'exit)
-		      (progn
-			(if notmuch-search-process-filter-data
-			    (insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
-			(insert "End of search results.")
-			(unless (= exit-status 0)
-			  (insert (format " (process returned %d)" exit-status)))
-			(insert "\n")
-			(if (and atbob
-				 (not (string= notmuch-search-target-thread "found")))
-			    (set 'never-found-target-thread t))))))
+		  (when (eq status 'exit)
+		    (if notmuch-search-process-filter-data
+			(insert (concat "Error: Unexpected output from notmuch search:\n" notmuch-search-process-filter-data)))
+		    (insert "End of search results.")
+		    (unless (= exit-status 0)
+		      (insert (format " (process returned %d)" exit-status)))
+		    (insert "\n")
+		    (if (and atbob
+			     (not (string= notmuch-search-target-thread "found")))
+			(set 'never-found-target-thread t)))))
 	      (when (and never-found-target-thread
 		       notmuch-search-target-line)
 		  (goto-char (point-min))
@@ -861,10 +860,9 @@ non-authors is found, assume that all of the authors match."
 			(put-text-property beg (point) 'notmuch-search-thread-id thread-id)
 			(put-text-property beg (point) 'notmuch-search-authors authors)
 			(put-text-property beg (point) 'notmuch-search-subject subject)
-			(if (string= thread-id notmuch-search-target-thread)
-			    (progn
-			      (set 'found-target beg)
-			      (set 'notmuch-search-target-thread "found"))))
+			(when (string= thread-id notmuch-search-target-thread)
+			  (set 'found-target beg)
+			  (set 'notmuch-search-target-thread "found")))
 		      (set 'line (match-end 0)))
 		  (set 'more nil)
 		  (while (and (< line (length string)) (= (elt string line) ?\n))
-- 
1.7.8.1

Re: [PATCH v2] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

On Wed,  1 Feb 2012 14:50:00 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.

I didn't check the indentation, but the sense of the changes is good.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[Dmitry Kurochkin]

On Wed, 01 Feb 2012 14:46:19 +0100, Pieter Praet <pieter@praet.org> wrote:
> On Mon, 30 Jan 2012 09:23:40 +0000, David Edmondson <dme@dme.org> wrote:
> > On Sat, 28 Jan 2012 13:14:45 -0400, David Bremner <david@tethera.net> wrote:
> > > On Sat, 28 Jan 2012 14:55:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> > > > On Jan 28, 2012 2:41 PM, "David Bremner" <david@tethera.net> wrote:
> > > > 
> > > > Sometimes someone (Dmitry?) sent patches that separated a small functional
> > > > change, and the big non-functional indentation change it caused,
> > > > separately. Would you prefer (or tolerate ;) that style?
> > > 
> > > Hmm, that might be nicer, I'm not 100% sure.
> > > 
> > > I wouldn't say it's mandatory for a patch like this (and I'd say other
> > > peoples views on what's easy to review are at least as important as mine
> > > here).
> > 
> > Each patch should be valid in the repository without any following
> > patches (preceding are obviously okay). Incorrect indentation would
> > disqualify a patch from being 'valid', so it shouldn't be accepted.
> 
> +1.
> 
> Indentation corrections should always be part of the same patch as
> the change(s) that invalidated the indentation in the first place.
> 
> Spotting the *actual* (non-indentation) changes is a non-issue
> when using `diff-refine-hunk' (or a wrapper thereof [1]).
> 

Must... resist... getting... into... this...

Re: [PATCH v2] emacs: globally replace non-branching "(if COND (progn ..." with "(when ..."

[David Bremner]

On Wed,  1 Feb 2012 14:50:00 +0100, Pieter Praet <pieter@praet.org> wrote:
> Less code, same results, without sacrificing readability.
> 

pushed.

d