[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Fix bug #11580

From: Sergio Durigan Junior
Subject: Re: [PATCH] Fix bug #11580
Date: Wed, 26 Sep 2012 15:18:40 -0300
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)

Hello Tassilo,

On Wednesday, September 26 2012, Tassilo Horn wrote:

> Sergio Durigan Junior <address@hidden> writes:
>> The attached patch fixes the bug listed on $SUBJECT.  Maybe there are
>> better ways to fix it, but a quick hack did the trick so I am sending
>> it for review.
> I don't use bbdb, so I've Cc-ed Roland who is the current bbdb
> maintainer.  Roland, the complete bug thread is here:
>   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11580
> In general, I think the patch isn't wrong, but somehow the whole
> function looks awkward to me.  COMMENTs inside:

Thanks for the review.

> --8<---------------cut here---------------start------------->8---
> (defun eudc-bbdb-format-record-as-result (record)
>   "Format the BBDB RECORD as a EUDC query result record.
> The record is filtered according to `eudc-bbdb-current-return-attributes'"
>   (let ((attrs (or eudc-bbdb-current-return-attributes
>                  '(firstname lastname aka company phones addresses net 
> notes)))
>       attr
>       eudc-rec
>       val)
>     (while (prog1
>              (setq attr (car attrs))
>            (setq attrs (cdr attrs)))
>       (cond
>        ((eq attr 'phones)
>       (setq val (eudc-bbdb-extract-phones record)))
>        ((eq attr 'addresses)
>       (setq val (eudc-bbdb-extract-addresses record)))
>        ((memq attr '(firstname lastname aka company net notes))
>       (setq val (eval
>                  (list (intern
>                         (concat "bbdb-record-"
>                                 (symbol-name attr)))
>                        'record))))
>        (t
>         ;; COMMENT1: Shouldn't that be (error "Unknown BBDB attribute")?
>         ;; Now, we'll enter the case of COMMENT3 with that val.

Agreed.  There is no point in entering (cond) with such wrong value.

>       (setq val "Unknown BBDB attribute")))
>       ;; COMMENT2: Your change, basically ok.  Before it was just (if val ...

Ok, thanks.  While waiting for the review, I thought a little more about
it and maybe it is worth to put that extra check inside (cond), instead
of inside the (if).

Anyway, I am sending a new version of the patch to fix this (and also
address your comments).

>       (if (and val (or (listp val) (not (string= val ""))))
>       (cond
>        ((memq attr '(phones addresses))
>         (setq eudc-rec (append val eudc-rec)))
>        ((and (listp val)
>              (= 1 (length val)))
>         (setq eudc-rec (cons (cons attr (car val)) eudc-rec)))
>          ;; COMMENT3: Don't we need to distinguish between a list of
>          ;; length > 0 and a string of length > 0? Or can't that happen?

I don't know the answer to this one.  In either case, I am checking to
see if `val' is not a list.

WDYT of the new patch below?



=== modified file 'lisp/net/eudcb-bbdb.el'
--- lisp/net/eudcb-bbdb.el      2012-01-19 07:21:25 +0000
+++ lisp/net/eudcb-bbdb.el      2012-09-26 18:14:52 +0000
@@ -166,7 +166,7 @@
                                  (symbol-name attr)))
-       (setq val "Unknown BBDB attribute")))
+       (error "Unknown BBDB attribute")))
       (if val
         ((memq attr '(phones addresses))
@@ -176,6 +176,8 @@
          (setq eudc-rec (cons (cons attr (car val)) eudc-rec)))
         ((> (length val) 0)
          (setq eudc-rec (cons (cons attr val) eudc-rec)))
+        ((and (not (listp val)) (string= val ""))
+         nil) ; Do nothing
          (error "Unexpected attribute value")))))
     (nreverse eudc-rec)))

reply via email to

[Prev in Thread] Current Thread [Next in Thread]