bug#20148: 24.4: function `bookmark-get-bookmark'

bug#20148: 24.4: function `bookmark-get-bookmark'

[Boruch Baum]

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

Function `bookmark-get-bookmark' ...

1] tests for the existence of a bookmark by checking whether the input
string matches a cons cell. This casts a net much wider than desirable,
and might better be (assoc bookmark-name-or-record bookmark-alist).

2] returns only a string value on error, without passing nil back to the
calling function.

My usage case is that I was scripting a function to find org-mode files
in a directory tree, and add them to a bookmark list if they aren't
already bookmarked according to my naming convention. Originally, I had
tried checking for the existence of a bookmark using a few of the
bookmark.el functions, such as bookmark-get-bookmark, but that looked
inconvenient because they returned string values on failure.

-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20148: 24.4: function `bookmark-get-bookmark'

[Drew Adams]

> Function `bookmark-get-bookmark' ...
> 
> 1] tests for the existence of a bookmark by checking whether the input
> string matches a cons cell. This casts a net much wider than desirable,
> and might better be (assoc bookmark-name-or-record bookmark-alist).

See the actual code.

> 2] returns only a string value on error, without passing nil back to the
> calling function.
> 
> My usage case is that I was scripting a function to find org-mode files
> in a directory tree, and add them to a bookmark list if they aren't
> already bookmarked according to my naming convention. Originally, I had
> tried checking for the existence of a bookmark using a few of the
> bookmark.el functions, such as bookmark-get-bookmark, but that looked
> inconvenient because they returned string values on failure.

See the actual code.

The doc string neglects to mention the optional argument.  It should
say something like this (taken from Bookmark+):

  Non-nil optional arg NOERROR means return nil if BOOKMARK is not a
  valid bookmark.  If NOERROR is nil then raise an error in this case.

bug#20148: 24.4: function `bookmark-get-bookmark'

[Boruch Baum]

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

Hello Drew. Apology for the inconvenience.

On 03/20/2015 01:04 AM, Drew Adams wrote:
>> Function `bookmark-get-bookmark' ...
>>
>> 1] tests for the existence of a bookmark by checking whether the input
>> string matches a cons cell. This casts a net much wider than desirable,
>> and might better be (assoc bookmark-name-or-record bookmark-alist).
> 
> See the actual code.
You're right. You could have just said 'line 336, not 335'.

> 
>> 2] returns only a string value on error, without passing nil back to the
>> calling function.
>>
>> My usage case is that I was scripting a function to find org-mode files
>> in a directory tree, and add them to a bookmark list if they aren't
>> already bookmarked according to my naming convention. Originally, I had
>> tried checking for the existence of a bookmark using a few of the
>> bookmark.el functions, such as bookmark-get-bookmark, but that looked
>> inconvenient because they returned string values on failure.
> 
> See the actual code.
> 
> The doc string neglects to mention the optional argument.  It should
> say something like this (taken from Bookmark+):
> 
>   Non-nil optional arg NOERROR means return nil if BOOKMARK is not a
>   valid bookmark.  If NOERROR is nil then raise an error in this case.
Missed that also.

-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20148: 24.4: function `bookmark-get-bookmark'

[Stefan Monnier]

Does that mean the bug can be closed?


        Stefan


>>>>> "Boruch" == Boruch Baum <boruch_baum@gmx.com> writes:

> Hello Drew. Apology for the inconvenience.
> On 03/20/2015 01:04 AM, Drew Adams wrote:
>>> Function `bookmark-get-bookmark' ...
>>> 
>>> 1] tests for the existence of a bookmark by checking whether the input
>>> string matches a cons cell. This casts a net much wider than desirable,
>>> and might better be (assoc bookmark-name-or-record bookmark-alist).
>> 
>> See the actual code.
> You're right. You could have just said 'line 336, not 335'.

>> 
>>> 2] returns only a string value on error, without passing nil back to the
>>> calling function.
>>> 
>>> My usage case is that I was scripting a function to find org-mode files
>>> in a directory tree, and add them to a bookmark list if they aren't
>>> already bookmarked according to my naming convention. Originally, I had
>>> tried checking for the existence of a bookmark using a few of the
>>> bookmark.el functions, such as bookmark-get-bookmark, but that looked
>>> inconvenient because they returned string values on failure.
>> 
>> See the actual code.
>> 
>> The doc string neglects to mention the optional argument.  It should
>> say something like this (taken from Bookmark+):
>> 
>> Non-nil optional arg NOERROR means return nil if BOOKMARK is not a
>> valid bookmark.  If NOERROR is nil then raise an error in this case.
> Missed that also.

> -- 
> hkp://keys.gnupg.net
> CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

bug#20148: 24.4: function `bookmark-get-bookmark'

[Boruch Baum]

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

On 03/20/2015 10:31 AM, Stefan Monnier wrote:
> Does that mean the bug can be closed?
Yes. Though, you might want to tweak the documentation, per Drew's
observation.

>> On 03/20/2015 01:04 AM, Drew Adams wrote:
>>> The doc string neglects to mention the optional argument.  It should
>>> say something like this (taken from Bookmark+):
>>>
>>> Non-nil optional arg NOERROR means return nil if BOOKMARK is not a
>>> valid bookmark.  If NOERROR is nil then raise an error in this case.

-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#20148: 24.4: function `bookmark-get-bookmark'

[Stefan Kangas]

[-- Attachment #1.1: Type: text/plain, Size: 178 bytes --]

The attached patch fixes the docstring as suggested above.

From the above discussion, I believe this bug report can be closed once
this has
been applied.

Thanks,
Stefan Kangas

[-- Attachment #1.2: Type: text/html, Size: 298 bytes --]

[-- Attachment #2: fix-docstring-of-bookmark-get-bookmark.patch --]
[-- Type: text/x-patch, Size: 987 bytes --]

From a874f231ba0216ca2c3feedaae8ddb32f378f80e Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 26 May 2019 12:04:56 +0200
Subject: [PATCH] Fix docstring of bookmark-get-bookmark

* bookmark.el (bookmark-get-bookmark): Fix docstring.
---
 lisp/bookmark.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index b1fe690dac..5c14f705eb 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -334,7 +334,9 @@ bookmark-get-bookmark
 If BOOKMARK-NAME-OR-RECORD is a string, look for the corresponding
 bookmark record in `bookmark-alist'; return it if found, otherwise
 error.  Else if BOOKMARK-NAME-OR-RECORD is already a bookmark record,
-just return it."
+just return it.
+Signal an error if bookmark is invalid.  If optional argument
+NOERROR is non-nil, return nil instead."
   (cond
    ((consp bookmark-name-or-record) bookmark-name-or-record)
    ((stringp bookmark-name-or-record)
-- 
2.11.0

bug#20148: 24.4: function `bookmark-get-bookmark'

[Basil L. Contovounesios]

tags 20148 fixed
close 20148 26.3
quit

> From a874f231ba0216ca2c3feedaae8ddb32f378f80e Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 26 May 2019 12:04:56 +0200
> Subject: [PATCH] Fix docstring of bookmark-get-bookmark
>
> * bookmark.el (bookmark-get-bookmark): Fix docstring.
> ---
>  lisp/bookmark.el | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/bookmark.el b/lisp/bookmark.el
> index b1fe690dac..5c14f705eb 100644
> --- a/lisp/bookmark.el
> +++ b/lisp/bookmark.el
> @@ -334,7 +334,9 @@ bookmark-get-bookmark
>  If BOOKMARK-NAME-OR-RECORD is a string, look for the corresponding
>  bookmark record in `bookmark-alist'; return it if found, otherwise
>  error.  Else if BOOKMARK-NAME-OR-RECORD is already a bookmark record,
> -just return it."
> +just return it.
> +Signal an error if bookmark is invalid.  If optional argument
> +NOERROR is non-nil, return nil instead."
>    (cond
>     ((consp bookmark-name-or-record) bookmark-name-or-record)
>     ((stringp bookmark-name-or-record)

The docstring already mentioned signalling an error, so I tweaked your
patch to include only the second of the two new sentences, as well as to
mention the full relative file name (lisp/bookmark.el) and the bug
reference (bug#20148) in the commit log, and pushed to emacs-26[1], and
am therefore closing this report.

[1: 1b2f83bb79]: Fix docstring of bookmark-get-bookmark
  2019-05-30 13:32:10 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1b2f83bb7984269b92eb0afb323e3e74baf87068

Thanks,

-- 
Basil