GNU bug report logs - #25032
25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

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 25032 in the body.
You can then email your comments to 25032 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: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Fri, 25 Nov 2016 16:21:12 -0800 (PST)

1. Why is "internal" function `bookmark-set-internal' a _command_
   (interactive)?  What sense does an "internal" command make?  It seems
   that the `interactive' spec here was a copy+paste mistake.  It should
   be removed, I think.

2. The `interactive' spec for `bookmark-set-internal' seems wrong
   anyway.  NAME is the raw prefix arg?  And then NAME is simply taken
   as is (as STR) and compared using `string-equal'?  This cannot be
   correct.

3. Similarly, why does the doc string of `bookmark-set-internal' say
   "_Interactively_..."?  It should just say that it prompts for a
   bookmark name and then...  And "error" is not easily and commonly
   understood as a verb - use "raise an error" instead.

4. `bookmark-set-internal' should preferably not accept both args PROMPT
   and NAME.  If NAME is present (e.g. for non-interactive use of
   `bookmark-set') then PROMPT makes no sense and is not used (and the
   doc string is wrong about PROMPT in that case).
   
5. In `bookmark-set-internal', a nil third arg should have been used to
   mean overwrite, not raise an error, as overwriting is still, and
   always has been, the default behavior of `bookmark-set'.  You should
   have introduced the new value `error', not the new value `overwrite',
   and kept the default (nil) behavior as overwriting.

   You will no doubt argue that this does not matter because
   `bookmark-set-internal' is "internal".  But the main command is
   still, and should still be `bookmark-set'.  `bookmark-set-internal'
   should reflect _its_ behavior for the default case (nil).

6. Error messages, like other messages, should not end with a period.

No doubt it is "too late" for #4 and #5.  Hopefully not for the rest.


In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-09-17 built on LAPHROAIG
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
 'configure --without-dbus --without-compress-install CFLAGS=-static'

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

From: Stefan Kangas <stefan <at> marxist.se>
To: 25032 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Wed, 12 Jun 2019 11:09:33 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

> 1. Why is "internal" function `bookmark-set-internal' a _command_
>    (interactive)?  What sense does an "internal" command make?  It seems
>    that the `interactive' spec here was a copy+paste mistake.  It should
>    be removed, I think.
>
> 2. The `interactive' spec for `bookmark-set-internal' seems wrong
>    anyway.  NAME is the raw prefix arg?  And then NAME is simply taken
>    as is (as STR) and compared using `string-equal'?  This cannot be
>    correct.

This has now been fixed.  See bug#36121.

> 6. Error messages, like other messages, should not end with a period.

This too has been fixed.  See bug#35916.

Best regards,
Stefan Kangas

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 25032 <at> debbugs.gnu.org
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Tue, 2 Jul 2019 07:23:27 +0200

Drew Adams <drew.adams <at> oracle.com> writes:

> 3. Similarly, why does the doc string of `bookmark-set-internal' say
>    "_Interactively_..."?  It should just say that it prompts for a
>    bookmark name and then...  And "error" is not easily and commonly
>    understood as a verb - use "raise an error" instead.

I have attached a patch which tries to improve the doc string.

> 4. `bookmark-set-internal' should preferably not accept both args PROMPT
>    and NAME.

See below.

>               If NAME is present (e.g. for non-interactive use of
>    `bookmark-set') then PROMPT makes no sense and is not used (and the
>    doc string is wrong about PROMPT in that case).

Please see if my above patch does not improve the situation.

> 5. In `bookmark-set-internal', a nil third arg should have been used to
>    mean overwrite, not raise an error, as overwriting is still, and
>    always has been, the default behavior of `bookmark-set'.  You should
>    have introduced the new value `error', not the new value `overwrite',
>    and kept the default (nil) behavior as overwriting.

I see your point.  But on the other hand a user can just use
bookmark-set and bookmark-set-overwrite, which are not advertised as
internal.

>    You will no doubt argue that this does not matter because
>    `bookmark-set-internal' is "internal".  But the main command is
>    still, and should still be `bookmark-set'.  `bookmark-set-internal'
>    should reflect _its_ behavior for the default case (nil).

I suspect that some of the things above will be a bit tricky to fix if
we still want to keep this as a general (internal) function used by
bookmark-set and bookmark-set-internal.  Which would be the main purpose
of this function, as I understand it.

The way it's written actually makes the implementation of the latter two
functions very straghtforward.  The code is not very hard to follow, in
my opinion.  But it's harder to think of a better alternative.

So, after we improve the doc string, this reviewer sees two options for
moving forward here:

1. Someone writes up a concrete suggestion.
2. We close this as wontfix and move on.

Thanks,
Stefan Kangas

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 25032 <at> debbugs.gnu.org
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Tue, 2 Jul 2019 07:27:34 +0200

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

Patch attached.

Thanks,
Stefan Kangas

[0001-lisp-bookmark.el-bookmark-set-internal-Doc-fix.-Bug-.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 25032 <at> debbugs.gnu.org, drew.adams <at> oracle.com
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Tue, 02 Jul 2019 19:48:34 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 2 Jul 2019 07:27:34 +0200
> Cc: 25032 <at> debbugs.gnu.org
> 
> +If OVERWRITE-OR-PUSH is nil, then raise an error if there is

We use "signal an error".

> +already a bookmark named NAME; if `overwrite', then replace any
> +existing bookmark if there is one; if `push' then push the new
> +bookmark onto the bookmark alist.

It is generally better to precede a description of several optional
behaviors with a short summary:

  OVERWRITE-OR-PUSH controls what happens if there is already a
  bookmark by that NAME: nil means signal an error, `overwrite' means
  replace the existing bookmark, `push' means ...

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25032 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Tue, 2 Jul 2019 19:07:57 +0200

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

Eli Zaretskii <eliz <at> gnu.org> writes:

> > +If OVERWRITE-OR-PUSH is nil, then raise an error if there is
>
> We use "signal an error".

Fixed.

> > +already a bookmark named NAME; if `overwrite', then replace any
> > +existing bookmark if there is one; if `push' then push the new
> > +bookmark onto the bookmark alist.
>
> It is generally better to precede a description of several optional
> behaviors with a short summary:
>
>   OVERWRITE-OR-PUSH controls what happens if there is already a
>   bookmark by that NAME: nil means signal an error, `overwrite' means
>   replace the existing bookmark, `push' means ...

Fixed.  Although I chose the wording "with the same name" rather than
"by that NAME", since the name could also come from the prompt.

Thanks for the review; attached an updated version.

Best regards,
Stefan Kangas

[0001-lisp-bookmark.el-bookmark-set-internal-Doc-fix.-Bug-2.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: drew.adams <at> oracle.com, 25032-done <at> debbugs.gnu.org
Subject: Re: bug#25032: 25.1;
 `bookmark-set-internal', `bookmark-set-no-overwrite'
Date: Sat, 06 Jul 2019 11:55:53 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 2 Jul 2019 19:07:57 +0200
> Cc: Drew Adams <drew.adams <at> oracle.com>, 25032 <at> debbugs.gnu.org
> 
> Thanks for the review; attached an updated version.

Thanks, I pushed this to the master branch.

Previous Next

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