[PATCH] emacs: replace (funcall 'foo ...) with (foo ...)

[PATCH] emacs: replace (funcall 'foo ...) with (foo ...)

[david]

From: David Bremner <bremner@debian.org>

I can't see any benefit to the funcall, and it looks like the result
of cut-and-paste from some code that actually used a variable for the
function to call.
---

Mark and I were discussing some style issues in the context of pick,
and it seems to me that some of the elisp style in the current code
could be improved. I don't claim to be an elisp style expert by any
stretch of the imagination. It would be great if some experts could
add a section to devel/STYLE about elisp.


 emacs/notmuch-message.el | 2 +-
 emacs/notmuch-show.el    | 6 +++---
 emacs/notmuch.el         | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-message.el b/emacs/notmuch-message.el
index 4dc4883..914bdd1 100644
--- a/emacs/notmuch-message.el
+++ b/emacs/notmuch-message.el
@@ -40,7 +40,7 @@ the \"inbox\" and \"todo\" tags, you would set:
   ;; get the in-reply-to header and parse it for the message id.
   (let ((rep (mail-header-parse-addresses (message-field-value "In-Reply-To"))))
     (when (and notmuch-message-replied-tags rep)
-      (funcall 'notmuch-tag (notmuch-id-to-query (car (car rep)))
+      (notmuch-tag (notmuch-id-to-query (car (car rep)))
 	       (notmuch-tag-change-list notmuch-message-replied-tags)))))
 
 (add-hook 'message-send-hook 'notmuch-message-mark-replied)
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 613e666..18b4671 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1726,7 +1726,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
   (let* ((current-tags (notmuch-show-get-tags))
 	 (new-tags (notmuch-update-tags current-tags tag-changes)))
     (unless (equal current-tags new-tags)
-      (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes)
+      (notmuch-tag (notmuch-show-get-message-id) tag-changes)
       (notmuch-show-set-tags new-tags))))
 
 (defun notmuch-show-tag (&optional tag-changes)
@@ -1734,7 +1734,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
   (interactive)
-  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes))
+  (setq tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
   (let* ((current-tags (notmuch-show-get-tags))
 	 (new-tags (notmuch-update-tags current-tags tag-changes)))
     (unless (equal current-tags new-tags)
@@ -1745,7 +1745,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
   (interactive)
-  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
+  (setq tag-changes (notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
   (notmuch-show-mapc
    (lambda ()
      (let* ((current-tags (notmuch-show-get-tags))
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 5a8c957..af107e2 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -562,7 +562,7 @@ will be signaled."
 (defun notmuch-search-tag-region (beg end &optional tag-changes)
   "Change tags for threads in the given region."
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
-    (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
+    (setq tag-changes (notmuch-tag search-string tag-changes))
     (notmuch-search-foreach-result beg end
       (lambda (pos)
 	(notmuch-search-set-tags
@@ -576,7 +576,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
   (interactive)
   (let* ((beg (if (region-active-p) (region-beginning) (point)))
 	 (end (if (region-active-p) (region-end) (point))))
-    (funcall 'notmuch-search-tag-region beg end tag-changes)))
+    (notmuch-search-tag-region beg end tag-changes)))
 
 (defun notmuch-search-add-tag ()
   "Same as `notmuch-search-tag' but sets initial input to '+'."
-- 
1.8.2.rc2

Re: [PATCH] emacs: replace (funcall 'foo ...) with (foo ...)

[Tomi Ollila]

On Sun, Jun 02 2013, david@tethera.net wrote:

> From: David Bremner <bremner@debian.org>
>
> I can't see any benefit to the funcall, and it looks like the result
> of cut-and-paste from some code that actually used a variable for the
> function to call.
> ---

Looks sensible to me.

Tomi


>
> Mark and I were discussing some style issues in the context of pick,
> and it seems to me that some of the elisp style in the current code
> could be improved. I don't claim to be an elisp style expert by any
> stretch of the imagination. It would be great if some experts could
> add a section to devel/STYLE about elisp.
>
>
>  emacs/notmuch-message.el | 2 +-
>  emacs/notmuch-show.el    | 6 +++---
>  emacs/notmuch.el         | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-message.el b/emacs/notmuch-message.el
> index 4dc4883..914bdd1 100644
> --- a/emacs/notmuch-message.el
> +++ b/emacs/notmuch-message.el
> @@ -40,7 +40,7 @@ the \"inbox\" and \"todo\" tags, you would set:
>    ;; get the in-reply-to header and parse it for the message id.
>    (let ((rep (mail-header-parse-addresses (message-field-value "In-Reply-To"))))
>      (when (and notmuch-message-replied-tags rep)
> -      (funcall 'notmuch-tag (notmuch-id-to-query (car (car rep)))
> +      (notmuch-tag (notmuch-id-to-query (car (car rep)))
>  	       (notmuch-tag-change-list notmuch-message-replied-tags)))))
>  
>  (add-hook 'message-send-hook 'notmuch-message-mark-replied)
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 613e666..18b4671 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1726,7 +1726,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
>    (let* ((current-tags (notmuch-show-get-tags))
>  	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>      (unless (equal current-tags new-tags)
> -      (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes)
> +      (notmuch-tag (notmuch-show-get-message-id) tag-changes)
>        (notmuch-show-set-tags new-tags))))
>  
>  (defun notmuch-show-tag (&optional tag-changes)
> @@ -1734,7 +1734,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes))
> +  (setq tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
>    (let* ((current-tags (notmuch-show-get-tags))
>  	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>      (unless (equal current-tags new-tags)
> @@ -1745,7 +1745,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
> +  (setq tag-changes (notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
>    (notmuch-show-mapc
>     (lambda ()
>       (let* ((current-tags (notmuch-show-get-tags))
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 5a8c957..af107e2 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -562,7 +562,7 @@ will be signaled."
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
>    (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
> -    (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
> +    (setq tag-changes (notmuch-tag search-string tag-changes))
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
>  	(notmuch-search-set-tags
> @@ -576,7 +576,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
>    (let* ((beg (if (region-active-p) (region-beginning) (point)))
>  	 (end (if (region-active-p) (region-end) (point))))
> -    (funcall 'notmuch-search-tag-region beg end tag-changes)))
> +    (notmuch-search-tag-region beg end tag-changes)))
>  
>  (defun notmuch-search-add-tag ()
>    "Same as `notmuch-search-tag' but sets initial input to '+'."
> -- 
> 1.8.2.rc2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: replace setq + let with let*

[david]

From: David Bremner <bremner@debian.org>

I found several places where a setq is immediately followed by a let
or a let*. This seems to be the pessimal combination, with the
implicit scope of the setq combined with the extra indentation of the let.
I combined these cases into a single let* which I think is easier to read.
In two places I turned a single clause let into a let*.
---
 emacs/notmuch-hello.el | 4 ++--
 emacs/notmuch-show.el  | 4 ++--
 emacs/notmuch.el       | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index c1c6f4b..15e3614 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -260,8 +260,8 @@ afterwards.")
 (defun notmuch-hello-search (&optional search)
   (interactive)
   (unless (null search)
-    (setq search (notmuch-hello-trim search))
-    (let ((history-delete-duplicates t))
+    (let* ((search (notmuch-hello-trim search))
+	   (history-delete-duplicates t))
       (add-to-history 'notmuch-search-history search)))
   (notmuch-search search notmuch-search-oldest-first nil nil
 		  #'notmuch-hello-search-continuation))
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 18b4671..e8c8343 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1734,8 +1734,8 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
 
 See `notmuch-tag' for information on the format of TAG-CHANGES."
   (interactive)
-  (setq tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
-  (let* ((current-tags (notmuch-show-get-tags))
+  (let* ((tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
+	 (current-tags (notmuch-show-get-tags))
 	 (new-tags (notmuch-update-tags current-tags tag-changes)))
     (unless (equal current-tags new-tags)
       (notmuch-show-set-tags new-tags))))
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index af107e2..edb5a1c 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -904,9 +904,8 @@ Other optional parameters are used as follows:
   target-line: The line number to move to if the target thread does not
                appear in the search results."
   (interactive)
-  (if (null query)
-      (setq query (notmuch-read-query "Notmuch search: ")))
-  (let ((buffer (get-buffer-create (notmuch-search-buffer-title query))))
+  (let* ((query (or query (notmuch-read-query "Notmuch search: ")))
+	 (buffer (get-buffer-create (notmuch-search-buffer-title query))))
     (switch-to-buffer buffer)
     (notmuch-search-mode)
     ;; Don't track undo information for this buffer
-- 
1.8.2.rc2

Re: [PATCH] emacs: replace setq + let with let*

[Mark Walters]

On Sun, 02 Jun 2013, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> I found several places where a setq is immediately followed by a let
> or a let*. This seems to be the pessimal combination, with the
> implicit scope of the setq combined with the extra indentation of the let.
> I combined these cases into a single let* which I think is easier to read.
> In two places I turned a single clause let into a let*.
> ---
>  emacs/notmuch-hello.el | 4 ++--
>  emacs/notmuch-show.el  | 4 ++--
>  emacs/notmuch.el       | 5 ++---
>  3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index c1c6f4b..15e3614 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -260,8 +260,8 @@ afterwards.")
>  (defun notmuch-hello-search (&optional search)
>    (interactive)
>    (unless (null search)
> -    (setq search (notmuch-hello-trim search))
> -    (let ((history-delete-duplicates t))
> +    (let* ((search (notmuch-hello-trim search))
> +	   (history-delete-duplicates t))
>        (add-to-history 'notmuch-search-history search)))
>    (notmuch-search search notmuch-search-oldest-first nil nil

These look good to me except I don't see why the above is a let* not a
let?

Best wishes

Mark

>  		  #'notmuch-hello-search-continuation))
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 18b4671..e8c8343 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1734,8 +1734,8 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (setq tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
> -  (let* ((current-tags (notmuch-show-get-tags))
> +  (let* ((tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
> +	 (current-tags (notmuch-show-get-tags))
>  	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>      (unless (equal current-tags new-tags)
>        (notmuch-show-set-tags new-tags))))
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index af107e2..edb5a1c 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -904,9 +904,8 @@ Other optional parameters are used as follows:
>    target-line: The line number to move to if the target thread does not
>                 appear in the search results."
>    (interactive)
> -  (if (null query)
> -      (setq query (notmuch-read-query "Notmuch search: ")))
> -  (let ((buffer (get-buffer-create (notmuch-search-buffer-title query))))
> +  (let* ((query (or query (notmuch-read-query "Notmuch search: ")))
> +	 (buffer (get-buffer-create (notmuch-search-buffer-title query))))
>      (switch-to-buffer buffer)
>      (notmuch-search-mode)
>      ;; Don't track undo information for this buffer
> -- 
> 1.8.2.rc2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: replace (funcall 'foo ...) with (foo ...)

[Mark Walters]

This looks good to me +1

Mark

On Sun, 02 Jun 2013, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> I can't see any benefit to the funcall, and it looks like the result
> of cut-and-paste from some code that actually used a variable for the
> function to call.
> ---
>
> Mark and I were discussing some style issues in the context of pick,
> and it seems to me that some of the elisp style in the current code
> could be improved. I don't claim to be an elisp style expert by any
> stretch of the imagination. It would be great if some experts could
> add a section to devel/STYLE about elisp.
>
>
>  emacs/notmuch-message.el | 2 +-
>  emacs/notmuch-show.el    | 6 +++---
>  emacs/notmuch.el         | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-message.el b/emacs/notmuch-message.el
> index 4dc4883..914bdd1 100644
> --- a/emacs/notmuch-message.el
> +++ b/emacs/notmuch-message.el
> @@ -40,7 +40,7 @@ the \"inbox\" and \"todo\" tags, you would set:
>    ;; get the in-reply-to header and parse it for the message id.
>    (let ((rep (mail-header-parse-addresses (message-field-value "In-Reply-To"))))
>      (when (and notmuch-message-replied-tags rep)
> -      (funcall 'notmuch-tag (notmuch-id-to-query (car (car rep)))
> +      (notmuch-tag (notmuch-id-to-query (car (car rep)))
>  	       (notmuch-tag-change-list notmuch-message-replied-tags)))))
>  
>  (add-hook 'message-send-hook 'notmuch-message-mark-replied)
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 613e666..18b4671 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1726,7 +1726,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
>    (let* ((current-tags (notmuch-show-get-tags))
>  	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>      (unless (equal current-tags new-tags)
> -      (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes)
> +      (notmuch-tag (notmuch-show-get-message-id) tag-changes)
>        (notmuch-show-set-tags new-tags))))
>  
>  (defun notmuch-show-tag (&optional tag-changes)
> @@ -1734,7 +1734,7 @@ TAG-CHANGES is a list of tag operations for `notmuch-tag'."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-message-id) tag-changes))
> +  (setq tag-changes (notmuch-tag (notmuch-show-get-message-id) tag-changes))
>    (let* ((current-tags (notmuch-show-get-tags))
>  	 (new-tags (notmuch-update-tags current-tags tag-changes)))
>      (unless (equal current-tags new-tags)
> @@ -1745,7 +1745,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
>  
>  See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
> -  (setq tag-changes (funcall 'notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
> +  (setq tag-changes (notmuch-tag (notmuch-show-get-messages-ids-search) tag-changes))
>    (notmuch-show-mapc
>     (lambda ()
>       (let* ((current-tags (notmuch-show-get-tags))
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 5a8c957..af107e2 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -562,7 +562,7 @@ will be signaled."
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
>    (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
> -    (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
> +    (setq tag-changes (notmuch-tag search-string tag-changes))
>      (notmuch-search-foreach-result beg end
>        (lambda (pos)
>  	(notmuch-search-set-tags
> @@ -576,7 +576,7 @@ See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
>    (let* ((beg (if (region-active-p) (region-beginning) (point)))
>  	 (end (if (region-active-p) (region-end) (point))))
> -    (funcall 'notmuch-search-tag-region beg end tag-changes)))
> +    (notmuch-search-tag-region beg end tag-changes)))
>  
>  (defun notmuch-search-add-tag ()
>    "Same as `notmuch-search-tag' but sets initial input to '+'."
> -- 
> 1.8.2.rc2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: replace setq + let with let*

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>> -    (setq search (notmuch-hello-trim search))
>> -    (let ((history-delete-duplicates t))
>> +    (let* ((search (notmuch-hello-trim search))
>> +	   (history-delete-duplicates t))
>>        (add-to-history 'notmuch-search-history search)))
>>    (notmuch-search search notmuch-search-oldest-first nil nil
>
> These look good to me except I don't see why the above is a let* not a
> let?

I ended up dropping this hunk because I realized it introduced a bug;
the side-effect of the setq is needed outside the unless.

pushed the two patches (as amended) in this thread.

d