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

[Stefan Kangas <stefan@marxist.se>]

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