From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Karl Fogel Newsgroups: gmane.emacs.devel Subject: Re: Add new functions to mark/unmark/delete all bookmarks Date: Sat, 25 Jul 2020 15:36:41 -0500 Message-ID: <878sf7s55y.fsf@red-bean.com> References: <20200724005105.11f85d5f@pineapple> <87pn8ku3y9.fsf@red-bean.com> <20200725124618.49a073b1@pineapple> Reply-To: Karl Fogel Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="32940"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Matthew White Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Jul 25 22:37:24 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jzQv5-0008U9-9s for ged-emacs-devel@m.gmane-mx.org; Sat, 25 Jul 2020 22:37:23 +0200 Original-Received: from localhost ([::1]:55908 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jzQv4-0001Ae-A6 for ged-emacs-devel@m.gmane-mx.org; Sat, 25 Jul 2020 16:37:22 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:52850) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jzQuY-0000lS-V2 for emacs-devel@gnu.org; Sat, 25 Jul 2020 16:36:51 -0400 Original-Received: from newsp.red-bean.com ([45.79.25.59]:54830 helo=sanpietro.red-bean.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jzQuW-0002MF-Ti for emacs-devel@gnu.org; Sat, 25 Jul 2020 16:36:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=red-bean.com; s=202005newsp; h=Content-Type:MIME-Version:Message-ID: In-Reply-To:Date:Reply-To:References:Subject:Cc:To:From:Sender: Content-Transfer-Encoding:Content-ID:Content-Description; bh=dOOMpusvKTBLB/7Ph93IGIRlDhPd7ibND01LvXipFNE=; t=1595709405; x=1596919005; b=UjL+ZHAZMtzvHLnf6Ur6u5wc96gvpoh9b/otuLRw56LQjAP1rwAjMS53Oyvqqsbpy2Nz5bGc0z IC0gvHqlDwYAtK87J/YiI3/mU4tDwG51VjUBzENG/0qBvSC2GsSXmSfc/IrAJGuW5+TvIwTdl4Ti9 CAx5b6d2vNBTN8BzALqeXKE8U9l/tlkjowxIsaakOuxVxlWv7wHwgXWFM2w7eRxpk77YFxxl73Pze BvZD5Aa7rxG7m2B6o3zOb+eFQt8SB76bICtaK/fwklT9HWxec68QeDe2LGau1JQ7kUXg0IpvW+hX3 C3lCA6aTO0/t/qh1leRVWhdaDcVSeRfNBOrGg==; Original-Received: from 99-112-125-163.lightspeed.cicril.sbcglobal.net ([99.112.125.163]:55948 helo=floss) by sanpietro.red-bean.com with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jzQuR-0003b6-Ia; Sat, 25 Jul 2020 20:36:43 +0000 In-Reply-To: <20200725124618.49a073b1@pineapple> (Matthew White's message of "Sat, 25 Jul 2020 12:46:18 +0200") Received-SPF: pass client-ip=45.79.25.59; envelope-from=kfogel@red-bean.com; helo=sanpietro.red-bean.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/07/25 16:36:44 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:253236 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 25 Jul 2020, Matthew White wrote: >> Hi, Matthew. Thanks so much for this. I have reviewed the patch >> against 'master' (since it applies cleanly there, whereas it does not >> quite apply cleanly to 'emacs-27'). It is very good; I have just >> some minor comments below. > >Hi Karl. I'm flattered, thanks! > >I appreciate so much that you took the time to review my patch, and >I eagerly followed your suggestions! > >You will find attached both a patch that applies to master, and one >that applies to emacs-27... They are the same in term of changes. Thanks! I'll test against both branches, then. >I'm still waiting a reply to the copyright assignment... But in the >meantime we can freely discuss about the patch... > >> 1) It helps to include a summary line at the top of your commit >> message, followed by a blank line. The summary line should be >> limited to 50 characters or fewer, if possible. (This is all >> documented in the "Commit messages" section in the 'CONTRIBUTE' file >> in the top level of the Emacs source tree, by the way). In your >> case, the Subject header of your email almost does the job -- we can >> shorten it a bit to fit within 50 characters: >>=20 >> "Add ability to mark/unmark/delete all bookmarks" > >Thanks for pointing that out. Now that I read the 'CONTRIBUTE' file, >I have some questions (by the way, I needed a remainder to read...): > >1) I did commit a message with a summary line, as you described... > > [ Actually my line was the subject of the thread, yours (above) > is better, hence I switched to your idea, thanks again ;) ] > > And indeed I use `git format-patch -1` as suggested in 'CONTRIBUTE' > to produce a patch, but the command "strips" the summary line from > the commit message and uses it as 'Subject:' in the patch, where it > is prefixed with '[PATCH] ' if the option -k isn't also given... > > Once the patch is applied via `git am`, the 'Subject:' is put back > in the commit message as summary, without the '[PATCH] ' prefix. > > So, I wonder if you used `git am` to apply the patch...? Could you > tip me off, please? I didn't use 'git am' the previous time, but I will this time. I wish that the output produced by 'git format-patch' were both a little mo= re self-identifying and more self-explanatory. While I recognized the patc= h as such output, I forgot that the Subject line therefore would contain th= e first line of the commit message :-). Had I reviewed the commit message = after application, I would have seen that line appear, of course. Anyway, = you did the right thing here -- I just need to adjust my own process a bit. >2) With which quotes should I surround the variable/function names in > the commit message? `' or '' or =E2=80=98=E2=80=99? I could not figure = that out... I saw the followup discussion about this, with the answer being 'like so'. >[x] Don't repeat the file name each time in the commit message. Great. > ...And yes, I did take a look at the `git log`... Heh -- sorry there just happened to be a misleading commit message in the r= ecent history, yeah. >> Better to call ARG something like NO-CONFIRM, so its name reflects >> its purpose. >> >> And a suggestion: have the prompt say "Permanently delete all >> bookmarks? " instead. > >In `bookmark-delete-all': > >[x] Chage ARG to NO-CONFIRM. >[x] Prompt "Permanently delete all bookmarks? ". Thanks. >> Because of subtle assumptions associated with English word order, the >> current prompt ("Delete all bookmarks permanently? ") implies that >> the alternative might be to delete the bookmarks non-permanently. > >That's a very smart thought... I'm truly impressed! Well, I used to be an English teacher :-). >> Regarding the "FIXME" comment: that assumption is embedded throughout >> the bookmark-bmenu-* code, so I think there's no need for the "FIXME" >> (and indeed the comment may cause confusion, since its presence calls >> the assumption into question). >>=20 >> It might be good to make a followup change in which we document the >> last-line-is-empty assumption, but that's a separate change of course. > >[x] Remove the "FIXME" code comments. Thanks. >Following your guidelines is very instructing, Karl! It's a pleasure to review a well-done patch! >I also: > >[x] Change `nameN' and `fileN' into `name-N' and `file-N' (i.e. name0 > -> name-0) in all new functions and in the new test-list.bmk file. >[x] Indent new functions to split long lines into shorter ones. >[x] bookmark-tests.el (bookmark-test-bmenu-any-marks-list): New function > to test `bookmark-bmenu-any-marks' with a list of bookmarks. *nod* Got it; will take a look at those changes. >I hope to get back soon with good news about the copyright assignment. Sounds good. On both branches, I'll apply the corresponding new changesets, run 'make ch= eck', and test manually. I'll also look over the diffs again. I'll follow= up here after I've done so. I'm actually not sure yet which branch we should apply on -- I think 'emacs= -27' is the right answer, as per CONTRIBUTE, but if anyone knows better ple= ase say so. (I would make sure to use 'git am' to apply, to preserve attribution and hi= story information, of course.) Best regards, =2DKarl --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQJIBAEBCgAyFiEEsgfAWXyjn8Fyi3xbCKC3XMXtg0UFAl8cl9kUHGtmb2dlbEBy ZWQtYmVhbi5jb20ACgkQCKC3XMXtg0Vg9w/9HPRGmDZG/1aWgb7+/Cz8QE1oH05p 268//EDpYADh6ZcQ8rvfs4kuXUdLfDTDgTOXPLHsd15VwuzefKEqsQ38pYfhL4cj 9bwqLVaVk9Xi6LM1vOchO+BJeZLDPpqgZy6RtdvvNKrwavfk33LZ9XpBCgleLIAr wCMgvN7KrHqq7BGuByn2aHOAP+URqYU7xNJCh+hcRqKKrZyABCl603EXakukOrOB zi9h9JtbGQcVZOd8KfskhjkDUkMwusGQSZ9wSjZ//1IZzHiWcCMdp8LYGSBEX09S KnlVfRKDY/Hn6cnU4W4S/82v4tlcTWQZXiKEu0qBNsP4qTjn4aXnkluyPgEDajMo DiZtQyQBk8EQpghkIciM0P+e3yLFH3dMqtjJW8NqOJU2E4n+uSir9dxw7fgJ3j2Y DEryQAfnnXNzV8lNoupTVGVRgpNY/jH8EtI+DAxqtPPnMspGAhaXAvEwyZMmhf/6 QYSVam3Y6us53gyijzuszgcIBIOg68s61duIjZOWmFXk9bSgx5Nx1c2tXDvI7kBD LSNktDV3+jXxgcuK5D5fKqHDcp4TyzaCmLGn9JmzPw892FVjJVh/aVVARBdxjqCp xVZ2a71otozPKIDbusEFx2S+rBJWKKOW3tj9H57Kg7txQezXP55zzv4ND8EWyMyp /rJmhCDIekZIXnE= =NxPA -----END PGP SIGNATURE----- --=-=-=--