Re: Add new functions to mark/unmark/delete all bookmarks

[Karl Fogel]

I wrote:
>I will try again with the new the 'master' patch (both manual testing
>and 'make check').  I'll also do a top-to-bottom review again of the
>diff, just so we can be sure.

Hi, Matthew.  I have had a chance to test the new changes against the 'master' 
branch, both manually and with 'make check'.  Everything looks good.

I also re-reviewed the diff.  Actually, I diffed the *new* diff against your 
original diff from July 24th, since I'd already reviewed that one, and then I 
just reviewed the meta-diff :-).  Everything seems fine.  I see that in the 
tests you add some hyphens to bookmark names, e.g., "name0" to "name-0" (no 
problem).  You also started using the existing `bookmark-bmenu-any-marks' in 
the tests -- good thinking; I had forgotten that that function existed.

There is one very minor thing that I should have spotted before.  It is so 
minor that there is no need to post a new patch -- I can just add a fixup 
commit after applying your commit.  In the new function `bookmark-delete-all', 
the doc string says:

  "Permanently delete all bookmarks.
   Doesn't ask for confirmation if NO-CONFIRM is non-nil."

A more Emacs-y way to write this would be:

  "Permanently delete all bookmarks.
   If optional argument NO-CONFIRM is non-nil, don't ask for confirmation."

There are actually several changes there:

  - Say that the argument is optional.

  - Put the condition before the consequence.

  - Use an active verb rather than a semi-stative one ("don't" instead
    of "doesn't").  You did this in the first sentence ("delete all
    bookmarks"), just not in the second.  For more on doc strings, see
    
https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html
 .
    
Again, there is no need to redo the patch (unless you feel like it).  We can 
take care of it in a follow-up commit.

Please let us know when your paperwork is all done.  I'm looking forward to 
having this change in Emacs.

Best regards,
-Karl

signature.asc
Description: PGP signature