[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Karl Fogel
Subject: Re: Add new functions to mark/unmark/delete all bookmarks
Date: Sun, 02 Aug 2020 17:13:50 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

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
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,

Attachment: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]