bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Drew Adams]

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'

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Stefan Kangas]

Drew Adams <drew.adams@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

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Stefan Kangas]

Drew Adams <drew.adams@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

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 39 bytes --]

Patch attached.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-lisp-bookmark.el-bookmark-set-internal-Doc-fix.-Bug-.patch --]
[-- Type: application/octet-stream, Size: 2186 bytes --]

From 732f4922ae8e4a06f14f70d0485f9f816fb1f60b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Tue, 2 Jul 2019 07:09:13 +0200
Subject: [PATCH] * lisp/bookmark.el (bookmark-set-internal): Doc fix.
 (Bug#25032)

---
 lisp/bookmark.el | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bbef0a927d..c2899fa9f1 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -763,18 +763,23 @@ bookmark-minibuffer-read-name-map
     map))
 
 (defun bookmark-set-internal (prompt name overwrite-or-push)
-  "Interactively set a bookmark named NAME at the current location.
-
-Begin the interactive prompt with PROMPT, followed by a space, a
-generated default name in parentheses, a colon and a space.
-
-If OVERWRITE-OR-PUSH is nil, then error if there is 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.  The `push' behavior means that among
-bookmarks named NAME, this most recently set one becomes the one in
-effect, but the others are still there, in order, if the topmost one
-is ever deleted."
+  "Set a bookmark using specified NAME or prompting with PROMPT.
+The bookmark is set at the current location.
+
+If NAME is non-nil, use it as the name of the new bookmark.  In
+this case, the value of PROMPT is ignored.
+
+Otherwise, prompt the user for the bookmark name.  Begin the
+interactive prompt with PROMPT, followed by a space, a generated
+default name in parentheses, a colon and a space.
+
+If OVERWRITE-OR-PUSH is nil, then raise an error if there is
+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.  The `push' behavior means that
+among bookmarks named NAME, this most recently set one becomes
+the one in effect, but the others are still there, in order, if
+the topmost one is ever deleted."
   (unwind-protect
        (let* ((record (bookmark-make-record))
               ;; `defaults' is a transient element of the
-- 
2.21.0

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 2 Jul 2019 07:27:34 +0200
> Cc: 25032@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 ...

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

Eli Zaretskii <eliz@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

[-- Attachment #2: 0001-lisp-bookmark.el-bookmark-set-internal-Doc-fix.-Bug-2.patch --]
[-- Type: application/octet-stream, Size: 2199 bytes --]

From 7b72c3642f4cc017b1477b0377fb17526f0dc0f8 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Tue, 2 Jul 2019 07:09:13 +0200
Subject: [PATCH] * lisp/bookmark.el (bookmark-set-internal): Doc fix.
 (Bug#25032)

---
 lisp/bookmark.el | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bbef0a927d..e190649fd1 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -763,18 +763,23 @@ bookmark-minibuffer-read-name-map
     map))
 
 (defun bookmark-set-internal (prompt name overwrite-or-push)
-  "Interactively set a bookmark named NAME at the current location.
-
-Begin the interactive prompt with PROMPT, followed by a space, a
-generated default name in parentheses, a colon and a space.
-
-If OVERWRITE-OR-PUSH is nil, then error if there is 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.  The `push' behavior means that among
-bookmarks named NAME, this most recently set one becomes the one in
-effect, but the others are still there, in order, if the topmost one
-is ever deleted."
+  "Set a bookmark using specified NAME or prompting with PROMPT.
+The bookmark is set at the current location.
+
+If NAME is non-nil, use it as the name of the new bookmark.  In
+this case, the value of PROMPT is ignored.
+
+Otherwise, prompt the user for the bookmark name.  Begin the
+interactive prompt with PROMPT, followed by a space, a generated
+default name in parentheses, a colon and a space.
+
+OVERWRITE-OR-PUSH controls what happens if there is already a
+bookmark with the same name: nil means signal an error;
+`overwrite' means replace any existing bookmark; `push' means
+push the new bookmark onto the bookmark alist.  The `push'
+behavior means that among bookmarks with the same name, this most
+recently set one becomes the one in effect, but the others are
+still there, in order, if the topmost one is ever deleted."
   (unwind-protect
        (let* ((record (bookmark-make-record))
               ;; `defaults' is a transient element of the
-- 
2.21.0

bug#25032: 25.1; `bookmark-set-internal', `bookmark-set-no-overwrite'

[Eli Zaretskii]

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

Thanks, I pushed this to the master branch.