GNU bug report logs - #69666
[PATCH] (vtable-update-object): Make old-object argument optional

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 69666 in the body.
You can then email your comments to 69666 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Adam Porter <adam <at> alphapapa.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] (vtable-update-object): Make old-object argument optional
Date: Fri, 8 Mar 2024 23:51:33 -0600

[Message part 1 (text/plain, inline)]

Hi,

Please see the attached patch which makes `vtable-update-object' easier 
to use in the common case of updating an existing object's 
representation in a table (rather than replacing it with another object).

Thanks,
Adam

[0001-vtable-update-object-Make-old-object-argument-option.patch (text/x-patch, attachment)]

Message #8 received at 69666 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Adam Porter <adam <at> alphapapa.net>
Cc: 69666 <at> debbugs.gnu.org
Subject: Re: bug#69666: [PATCH] (vtable-update-object): Make old-object
 argument optional
Date: Thu, 14 Mar 2024 11:00:51 +0200

> Date: Fri, 8 Mar 2024 23:51:33 -0600
> From: Adam Porter <adam <at> alphapapa.net>
> 
> Please see the attached patch which makes `vtable-update-object' easier 
> to use in the common case of updating an existing object's 
> representation in a table (rather than replacing it with another object).

Thanks, I have some minor comments below.

> Subject: [PATCH] (vtable-update-object): Make old-object argument optional

Since this changes the API of a public function, we need to call this
out in NEWS.

> +@defun vtable-update-object table object &optional old-object
> +Change @var{old-object} into @var{object} in @var{table}; or, without
> +@var{old-object}, update existing @var{object} in @var{table}.  This
> +also updates the displayed table.

This is backwards: the documentation should first say what happens if
the function is called with just 2 arguments, and then what happens if
the 3rd one is supplied.  Like this:

  Update @var{object} in @var{table} and redisplay @var{table}.
  Optional argument @var{old-object}, if non-@code{nil}, means to
  change @var{old-object} into @var{object}.

> +(defun vtable-update-object (table object &optional old-object)
> +  "Replace OLD-OBJECT in TABLE with OBJECT.
> +Without OLD-OBJECT, just update existing OBJECT in TABLE."

Same here.

Message #11 received at 69666 <at> debbugs.gnu.org (full text, mbox):

From: Adam Porter <adam <at> alphapapa.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69666 <at> debbugs.gnu.org
Subject: Re: bug#69666: [PATCH] (vtable-update-object): Make old-object
 argument optional
Date: Fri, 15 Mar 2024 19:41:33 -0500

[Message part 1 (text/plain, inline)]

Hi Eli,

On 3/14/24 04:00, Eli Zaretskii wrote:
>> Date: Fri, 8 Mar 2024 23:51:33 -0600
>> From: Adam Porter <adam <at> alphapapa.net>
>>
>> Please see the attached patch which makes `vtable-update-object' easier
>> to use in the common case of updating an existing object's
>> representation in a table (rather than replacing it with another object).
> 
> Thanks, I have some minor comments below.

Thanks for your review.  Please see the attached patch which addresses 
those three items and is rebased on current master.  Please let me know 
if I need to make any further changes.

Thanks,
Adam

[0001-vtable-update-object-Make-old-object-argument-option.patch (text/x-patch, attachment)]

Message #14 received at 69666 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Adam Porter <adam <at> alphapapa.net>
Cc: 69666 <at> debbugs.gnu.org
Subject: Re: bug#69666: [PATCH] (vtable-update-object): Make old-object
 argument optional
Date: Sat, 16 Mar 2024 12:29:59 +0200

> Date: Fri, 15 Mar 2024 19:41:33 -0500
> Cc: 69666 <at> debbugs.gnu.org
> From: Adam Porter <adam <at> alphapapa.net>
> 
> > Thanks, I have some minor comments below.
> 
> Thanks for your review.  Please see the attached patch which addresses 
> those three items and is rebased on current master.  Please let me know 
> if I need to make any further changes.

Thanks, this LGTM.  Just a couple of minor nits:

> Subject: [PATCH] (vtable-update-object): Make old-object argument optional
>  (Bug#69666)

This is too long for the generated ChangeLog.  Since this just
reiterates what the log message below it says, I suggest:

  . make the heading more terse, like

     'vtable-update-object' can now be called with one argument

  . move the bug number to the actual log entry:

    * lisp/emacs-lisp/vtable.el (vtable-update-object): Make 'old-object'
    argument optional.  (Bug#69666)

> +(defun vtable-update-object (table object &optional old-object)
> +  "Update OBJECT's representation in TABLE.
> +When OLD-OBJECT is non-nil, replace OLD-OBJECT with OBJECT and display

"When" has a meaning of time, which is not what you mean here.  I
suggest to use "If" in these cases.

Message #17 received at 69666 <at> debbugs.gnu.org (full text, mbox):

From: Adam Porter <adam <at> alphapapa.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69666 <at> debbugs.gnu.org
Subject: Re: bug#69666: [PATCH] (vtable-update-object): Make old-object
 argument optional
Date: Sat, 16 Mar 2024 23:29:52 -0500

[Message part 1 (text/plain, inline)]

Hi Eli,

Thanks, please see the attached patch.

On 3/16/24 05:29, Eli Zaretskii wrote:

> Thanks, this LGTM.  Just a couple of minor nits:
> 
>> Subject: [PATCH] (vtable-update-object): Make old-object argument
>> optional (Bug#69666)
> 
> This is too long for the generated ChangeLog.  Since this just 
> reiterates what the log message below it says, I suggest:
> 
> . make the heading more terse, like
> 
> 'vtable-update-object' can now be called with one argument
> 
> . move the bug number to the actual log entry:
> 
> * lisp/emacs-lisp/vtable.el (vtable-update-object): Make
> 'old-object' argument optional.  (Bug#69666)

Done.

For future reference, is there a way to know whether the heading line is
too long for the ChangeLog entry, other than manually counting?

> "When" has a meaning of time, which is not what you mean here.  I 
> suggest to use "If" in these cases.

Done.

Also, please note that I just discovered another limitation of this
function, which I've filed as bug#69837.  Since it seems quite
important, I've also updated the patch to mention the limitation in the
docstring and manual.

Thanks,
Adam

[0001-vtable-update-object-can-now-be-called-with-one-argu.patch (text/x-patch, attachment)]

Message #22 received at 69666-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Adam Porter <adam <at> alphapapa.net>
Cc: 69666-done <at> debbugs.gnu.org
Subject: Re: bug#69666: [PATCH] (vtable-update-object): Make old-object
 argument optional
Date: Thu, 21 Mar 2024 12:44:36 +0200

> Date: Sat, 16 Mar 2024 23:29:52 -0500
> Cc: 69666 <at> debbugs.gnu.org
> From: Adam Porter <adam <at> alphapapa.net>
> 
> Thanks, please see the attached patch.

Thanks, installed, and closing the bug.

> For future reference, is there a way to know whether the heading line is
> too long for the ChangeLog entry, other than manually counting?

I suggest to arrange for fill-column to be set in the buffers where
you edit the log messages, then M-q or auto-fill-mode will do this for
you.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.