From: Sebastian Lipp <bacuh@riseup.net>
To: Karl Fogel <kfogel@red-bean.com>
Cc: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.
Date: Fri, 04 Jul 2014 11:25:38 +0200 [thread overview]
Message-ID: <87r4215vcd.fsf@verb.i-did-not-set--mail-host-address--so-tickle-me> (raw)
In-Reply-To: <87oax6vt4u.fsf@ktab.red-bean.com>
Karl Fogel <kfogel@red-bean.com> writes:
> Sebastian Lipp <bacuh@riseup.net> writes:
>>As I'd like to see this in notmuch I made the change. The patch is
>>attached. As it is my first contribution to notmuch at all: Just tell me
>>if I'm supposed to do it in any other way.
>
> I think your patch includes much more than just the above, though.
True. See below on that.
>>diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
>>index fa65cd5..ee7b169 100644
>>--- a/emacs/notmuch-address.el
>>+++ b/emacs/notmuch-address.el
>>@@ -113,6 +113,59 @@ to know how address selection is made by default."
>> (when (notmuch-address-locate-command notmuch-address-command)
>> (notmuch-address-message-insinuate))
>>
>>+;; functions to add sender / recipients to BBDB
>>+
>>+(defvar bbdb-get-addresses-headers)
>
> I think it's good to include an initial value (even an invalid
> placeholder one, if the real initialization has not happened yet), and a
> doc string. C-h f defvar RET will say more about how to do that.
I will read into this and get back to it.
>>+(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>>+
>>+(declare-function bbdb-get-addresses "bbdb-com"
>>+ (only-first-address
>>+ uninteresting-senders
>>+ get-header-content-function
>>+ &rest get-header-content-function-args))
>>+
>>+(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
>
> At this point, your patch has accomplished what Tomi originally
> suggested. But then the patch continues with what looks like
> substantive new code:
>
>>+(defun notmuch-bbdb/snarf-headers (headers)
>>+ ;; Helper function to avoid code duplication in the two below
>>+ ;; headers should have the same format as bbdb-get-addresses-headers
>>+
>>+ ;; bbdb-get-addresses reads these
>>+ ;; Ugh, pass-by-global
>>+ (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
>>+ (bbdb-get-addresses-headers headers) ; headers to read
>>+ (bbdb-gag-messages t)) ; suppress m/n processed message)
>>+ (bbdb-update-records addrs t t)))
>>+
>>+(defun notmuch-bbdb/snarf-from ()
>>+ "Import the sender of the current message into BBDB"
>>+ (interactive)
>>+ (notmuch-bbdb/snarf-headers
>>+ (list (assoc 'authors bbdb-get-addresses-headers))))
>>+
>>+(defun notmuch-bbdb/snarf-to ()
>>+ "Import all recipients of the current message into BBDB"
>>+ (interactive)
>>+ (notmuch-bbdb/snarf-headers
>>+ (list (assoc 'recipients bbdb-get-addresses-headers))))
>>+
>>+(defvar notmuch-bbdb/header-by-name
>>+ ;; both are case sensitive
>>+ '( ("From" . :From)
>>+ ("To" . :To)
>>+ ("CC" . :Cc)
>>+ ("BCC" . :Bcc)
>>+ ("Resent-From" . nil)
>>+ ("Reply-To" . nil)
>>+ ("Resent-To" . nil)
>>+ ("Resent-CC" . nil))
>>+ "Alist for dispatching header symbols as used by notmuch-show-get-header
>>+from strings as used by bbdb-get-addresses")
>>+
>>+(defun notmuch-bbdb/get-header-content (name)
>>+ (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
>
> It looks like new code, but I suspect what actually happened is that
> this is just the original code, somehow mis-expressed as "+" lines in
> the patch. Is that what happened?
No, it happened by intention because what is "original" to you is not
part of my freshly cloned notmuch. So I thought good practise is to
construct *one* patch that brings the already fixed feature to the
notmuch codebase to keep it clean.
If you like better, my next patch will only base Tomis and your
suggestions on top of the "original" patch.
basti
next prev parent reply other threads:[~2014-07-04 9:25 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-06 18:33 [PATCH] emacs: add missing paren to fix defun in notmuch-address.el Karl Fogel
2013-04-07 13:52 ` David Bremner
2013-04-07 21:50 ` Karl Fogel
2013-04-08 8:16 ` Tomi Ollila
2013-04-08 8:40 ` Jani Nikula
2013-04-08 10:50 ` David Bremner
2013-04-09 1:12 ` Jed Brown
2013-04-09 6:32 ` Tomi Ollila
2013-04-09 11:50 ` David Bremner
2013-04-09 12:12 ` Tomi Ollila
2014-07-04 0:38 ` Sebastian Lipp
2014-07-04 0:56 ` Karl Fogel
2014-07-04 9:25 ` Sebastian Lipp [this message]
2014-07-05 18:18 ` Karl Fogel
2014-07-06 20:41 ` Sebastian Lipp
2014-07-07 17:35 ` Karl Fogel
2014-07-06 20:37 ` Sebastian Lipp
2014-07-07 17:40 ` Karl Fogel
2014-07-08 8:56 ` Tomi Ollila
2014-07-08 17:59 ` Karl Fogel
2014-07-04 16:31 ` Tomi Ollila
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r4215vcd.fsf@verb.i-did-not-set--mail-host-address--so-tickle-me \
--to=bacuh@riseup.net \
--cc=kfogel@red-bean.com \
--cc=notmuch@notmuchmail.org \
--cc=tomi.ollila@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).