[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Tomi Ollila  writes:
>but the bbdb-* looks a bit too generic (goes deep into bbdb "namespace"
>in a file not part of bbdb -package)

Good point.  Stuff in notmuch should have a consistent prefix.  Prefixes
are Emacs' module system, after all :-).  (It could be `notmuch-bbdb-*'
or whatever, of course.)

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Tomi Ollila tomi.oll...@iki.fi writes:
but the bbdb-* looks a bit too generic (goes deep into bbdb namespace
in a file not part of bbdb -package)

Good point.  Stuff in notmuch should have a consistent prefix.  Prefixes
are Emacs' module system, after all :-).  (It could be `notmuch-bbdb-*'
or whatever, of course.)
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp  writes:
>> 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 hope I got that right now. (I've got no real clue of Lisp yet because
>I just recently switched to emacs partly because of notmuch. :)
>
>How about
>
>(defvar bbdb-get-addresses-headers nil
>  "List of Addresses to import into bbdb")

That's interesting -- from seeing the variable's name, I would not have
guessed that it is a list.

Can the doc string describe the exact format of the list, and something
about how the list is initialized and used?

In other words, if you were a programmer seeing this variable for the
first time, and you had only a basic working knowledge of what the
notmuch Elisp code does, what would you want to see in this doc string
to give you a good understanding of what this variable is, how it gets
set, and how it's used?

One test I often use for myself is that if after I've read the doc
string, I can look at any value and say whether or not it is a valid
value that this variable might hold (when the code is running), then the
doc string has done its job.

Best,
-Karl

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp  writes:
>Karl Fogel 
>> In general, supplying a log message with the patch with avoid such
>> confusion.  If there is some prose expressing what the change is
>> supposed to to, and giving any historical context (such as the mailing
>> list thread starting from last year), then it will be easy for any
>> reviewer to understand what the patch is intended to do, and check if it
>> actually does that.
>
>Okay, next time I will provide that information inline. Thought keeping
>References and In-Reply-To headers would be sufficient reference.

Well, when the patch is committed into a version control system, it's
going to need a log message (commit message) anyway.  Since that message
is considered part of the change -- anyone reading the change will start
by reading the commit message -- it's typical to just include it with
the diff.

The commit message can certainly include a reference to the email
thread; in fact, it's good if it does so.

Best,
?Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp ba...@riseup.net writes:
 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 hope I got that right now. (I've got no real clue of Lisp yet because
I just recently switched to emacs partly because of notmuch. :)

How about

(defvar bbdb-get-addresses-headers nil
  List of Addresses to import into bbdb)

That's interesting -- from seeing the variable's name, I would not have
guessed that it is a list.

Can the doc string describe the exact format of the list, and something
about how the list is initialized and used?

In other words, if you were a programmer seeing this variable for the
first time, and you had only a basic working knowledge of what the
notmuch Elisp code does, what would you want to see in this doc string
to give you a good understanding of what this variable is, how it gets
set, and how it's used?

One test I often use for myself is that if after I've read the doc
string, I can look at any value and say whether or not it is a valid
value that this variable might hold (when the code is running), then the
doc string has done its job.

Best,
-Karl
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp  writes:
>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.

Oh, I had thought the changes were already in the notmuch tree.  Now I
understand what you're saying, and yes, it makes sense.

In general, supplying a log message with the patch with avoid such
confusion.  If there is some prose expressing what the change is
supposed to to, and giving any historical context (such as the mailing
list thread starting from last year), then it will be easy for any
reviewer to understand what the patch is intended to do, and check if it
actually does that.

Best,
-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp ba...@riseup.net writes:
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.

Oh, I had thought the changes were already in the notmuch tree.  Now I
understand what you're saying, and yes, it makes sense.

In general, supplying a log message with the patch with avoid such
confusion.  If there is some prose expressing what the change is
supposed to to, and giving any historical context (such as the mailing
list thread starting from last year), then it will be easy for any
reviewer to understand what the patch is intended to do, and check if it
actually does that.

Best,
-Karl
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp ba...@riseup.net writes:
Tomi Ollila tomi.oll...@iki.fi writes:
 On Tue, Apr 09 2013, David Bremner wrote:
 There seems to be a few warnings:

 In notmuch-bbdb/snarf-from:
 notmuch-address.el:116:26:Warning: reference to free variable
 `bbdb-get-addresses-headers'

 In notmuch-bbdb/snarf-to:
 notmuch-address.el:122:29:Warning: reference to free variable
 `bbdb-get-addresses-headers'

 In end of data:
 notmuch-address.el:143:1:Warning: the following functions are not known to 
 be
 defined: bbdb-get-addresses, bbdb-update-records, 
 notmuch-show-get-header

 Do we need a few defvars?

 For the above set, something like:

 (defvar bbdb-get-addresses-headers)

 (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))

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.

(It helps to include a log message with the patch -- then people can see
what you intended the code change to be, and compare that against the
code change the patch actually makes.)

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.

(By the way, this isn't a user-customizeable variable, right?  If it
were, then `defcustom' would be better than `defvar'.)

+(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?

Best,
-Karl
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp  writes:
>Tomi Ollila  writes:
>> On Tue, Apr 09 2013, David Bremner wrote:
>>> There seems to be a few warnings:
>>>
>>> In notmuch-bbdb/snarf-from:
>>> notmuch-address.el:116:26:Warning: reference to free variable
>>> `bbdb-get-addresses-headers'
>>>
>>> In notmuch-bbdb/snarf-to:
>>> notmuch-address.el:122:29:Warning: reference to free variable
>>> `bbdb-get-addresses-headers'
>>>
>>> In end of data:
>>> notmuch-address.el:143:1:Warning: the following functions are not known to 
>>> be
>>> defined: bbdb-get-addresses, bbdb-update-records, 
>>> notmuch-show-get-header
>>>
>>> Do we need a few defvars?
>>
>> For the above set, something like:
>>
>> (defvar bbdb-get-addresses-headers)
>>
>> (declare-function notmuch-show-get-header "notmuch-show" (header  
>> props))
>>
>> (declare-function bbdb-get-addresses "bbdb-com" 
>>   (only-first-address
>>uninteresting-senders
>>get-header-content-function
>> get-header-content-function-args))
>>
>> (declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p 
>> offer-to-create))
>
>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.

(It helps to include a log message with the patch -- then people can see
what you intended the code change to be, and compare that against the
code change the patch actually makes.)

>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.

(By the way, this isn't a user-customizeable variable, right?  If it
were, then `defcustom' would be better than `defvar'.)

>+(declare-function notmuch-show-get-header "notmuch-show" (header  
>props))
>+
>+(declare-function bbdb-get-addresses "bbdb-com" 
>+  (only-first-address
>+   uninteresting-senders
>+   get-header-content-function
>+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?

Best,
-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

David Bremner da...@tethera.net writes:
Karl Fogel kfo...@red-bean.com writes:
 This patch fixes a trivial missing-paren problem in notmuch-address.el
 (and reindents the following defun accordingly).  I'm not subscribed
 to this list, so please keep me CC'd on any followups.

Dear Karl;

Thanks very much for the patch.

Since the offending commit is now reverted, it would be great if
somebody (TM) would combine your patch with  238bf4cb09.

Oh, it's trivial.  The problem with 238bf4cb09 was simply that the
function (defun) `notmuch-bbdb/snarf-headers' was missing a closing
paren.  A visible symptom of this was that the *next* defun after it,
`notmuch-bbdb/snarf-from', was spuriously indented inward.  If anyone
had tried reindenting further, all the code below it would also have
indented inward, making the problem more obvious.

So the solution is to:

  1) Re-apply the 238bf4cb09 patch

  2) Add a parenthesis to the end of `notmuch-bbdb/snarf-headers',
 such that the line (bbdb-update-records addrs t t)) becomes
 (bbdb-update-records addrs t t)))

  3) Unindent the function `notmuch-bbdb/snarf-from' immediately below
 there, which just means pulling each line leftward two spaces

  4) Commit, push, profit :-).

The above recipe is, of course, equivalent to re-applying the 238bf4cb09
patch, then applying my patch (4c74ad313f608f0834961c63c70d1f811ef103b7)
on top of it.  I'm not sure what the gitmost way to do that is, but if
you want I can simply submit a combined change whose commit message
makes clear what's going on.

-Karl
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

David Bremner  writes:
>Karl Fogel  writes:
>> This patch fixes a trivial missing-paren problem in notmuch-address.el
>> (and reindents the following defun accordingly).  I'm not subscribed
>> to this list, so please keep me CC'd on any followups.
>
>Dear Karl;
>
>Thanks very much for the patch.
>
>Since the offending commit is now reverted, it would be great if
>somebody (TM) would combine your patch with  238bf4cb09.

Oh, it's trivial.  The problem with 238bf4cb09 was simply that the
function (defun) `notmuch-bbdb/snarf-headers' was missing a closing
paren.  A visible symptom of this was that the *next* defun after it,
`notmuch-bbdb/snarf-from', was spuriously indented inward.  If anyone
had tried reindenting further, all the code below it would also have
indented inward, making the problem more obvious.

So the solution is to:

  1) Re-apply the 238bf4cb09 patch

  2) Add a parenthesis to the end of `notmuch-bbdb/snarf-headers',
 such that the line "(bbdb-update-records addrs t t))" becomes
 "(bbdb-update-records addrs t t)))"

  3) Unindent the function `notmuch-bbdb/snarf-from' immediately below
 there, which just means pulling each line leftward two spaces

  4) Commit, push, profit :-).

The above recipe is, of course, equivalent to re-applying the 238bf4cb09
patch, then applying my patch (4c74ad313f608f0834961c63c70d1f811ef103b7)
on top of it.  I'm not sure what the gitmost way to do that is, but if
you want I can simply submit a combined change whose commit message
makes clear what's going on.

-Karl

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

This patch fixes a trivial missing-paren problem in notmuch-address.el
(and reindents the following defun accordingly).  I'm not subscribed
to this list, so please keep me CC'd on any followups.

Best,
-Karl

From 4c74ad313f608f0834961c63c70d1f811ef103b7 Mon Sep 17 00:00:00 2001
From: Karl Fogel kfo...@red-bean.com
Date: Sat, 6 Apr 2013 13:21:54 -0400
Subject: [PATCH] emacs: add missing paren to close
 `notmuch-bbdb/snarf-headers' definition.

This unbreaks emacs/notmuch-address.el.  Without this fix,
the notmuch build process fails with this error:

  $ make
  ...
  In toplevel form:
  emacs/notmuch.el:56:1:Error: End of file during parsing: \
/path/to/source/tree/notmuch/emacs/notmuch-address.el
  make: *** [emacs/notmuch.elc] Error 1
  $
---
 emacs/notmuch-address.el |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 32c8490..4eda098 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -107,13 +107,13 @@ line.
   (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))
+(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-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
-- 
1.7.10.4

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

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

This patch fixes a trivial missing-paren problem in notmuch-address.el
(and reindents the following defun accordingly).  I'm not subscribed
to this list, so please keep me CC'd on any followups.

Best,
-Karl

-- next part --
A non-text attachment was scrubbed...
Name: 0001-emacs-add-missing-paren-to-close-notmuch-bbdb-snarf-.patch
Type: text/x-diff
Size: 1620 bytes
Desc: Trivial fix to restore missing paren in notmuch-address.el.
URL: