From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Martin Blais Newsgroups: gmane.emacs.bugs Subject: bug#6531: patch for rst.el updated (patch format revised) Date: Thu, 12 Apr 2012 23:13:51 -0400 Message-ID: <1334286831.24130.140661061773401.3D5346C2@webmail.messagingengine.com> References: <4C28AA99.7010900@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1334289936 14255 80.91.229.3 (13 Apr 2012 04:05:36 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 13 Apr 2012 04:05:36 +0000 (UTC) Cc: 6531@debbugs.gnu.org, Stefan Merten , David Goodger To: Stefan Monnier , "Wei-Wei Guo" Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Apr 13 06:05:34 2012 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1SIXlN-00050o-Gi for geb-bug-gnu-emacs@m.gmane.org; Fri, 13 Apr 2012 06:05:33 +0200 Original-Received: from localhost ([::1]:58614 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIXlM-0005zN-IP for geb-bug-gnu-emacs@m.gmane.org; Fri, 13 Apr 2012 00:05:32 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:54628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIXji-0005rA-8f for bug-gnu-emacs@gnu.org; Fri, 13 Apr 2012 00:03:51 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SIXje-0008DS-Og for bug-gnu-emacs@gnu.org; Fri, 13 Apr 2012 00:03:49 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:58892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIXje-0008DA-HH for bug-gnu-emacs@gnu.org; Fri, 13 Apr 2012 00:03:46 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.72) (envelope-from ) id 1SIXkr-0000le-RG for bug-gnu-emacs@gnu.org; Fri, 13 Apr 2012 00:05:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Martin Blais Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 13 Apr 2012 04:05:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 6531 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 6531-submit@debbugs.gnu.org id=B6531.13342898682906 (code B ref 6531); Fri, 13 Apr 2012 04:05:01 +0000 Original-Received: (at 6531) by debbugs.gnu.org; 13 Apr 2012 04:04:28 +0000 Original-Received: from localhost ([127.0.0.1]:55430 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1SIXkI-0000ko-HJ for submit@debbugs.gnu.org; Fri, 13 Apr 2012 00:04:27 -0400 Original-Received: from out1-smtp.messagingengine.com ([66.111.4.25]:33419) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1SIWya-00084V-Hy for 6531@debbugs.gnu.org; Thu, 12 Apr 2012 23:15:10 -0400 Original-Received: from compute4.internal (compute4.nyi.mail.srv.osa [10.202.2.44]) by gateway1.nyi.mail.srv.osa (Postfix) with ESMTP id 1AC4021850; Thu, 12 Apr 2012 23:13:52 -0400 (EDT) Original-Received: from betaweb1.nyi.mail.srv.osa ([10.202.2.10]) by compute4.internal (MEProxy); Thu, 12 Apr 2012 23:13:52 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=message-id:from:to:cc:mime-version :content-transfer-encoding:content-type:in-reply-to:references :subject:date; s=smtpout; bh=kj38+YTWhoKZ9uwZod/Ku1WrYhY=; b=S5x PdHBe3jekgsdVNzmVvskRd7mFziGtlAJn5jmmtCs7EdRfynNVj4PuvBPWNYIVGtI PcKMZJKMv93lW9jW9MsZzWKiSVtJftv5s9NkUnyxHqV0VM2GIDE5u1X+aTHS5SSP X7asSHOXpkg44xIrdgoKUgu1Uhk00kxaq3E3wXbM= Original-Received: by betaweb1.nyi.mail.srv.osa (Postfix, from userid 99) id DD62962810C; Thu, 12 Apr 2012 23:13:51 -0400 (EDT) X-Sasl-Enc: y7pFrMOX1ykNOqmQZgvj1Vp0GauEk6vTvWwq+BIxcfQS 1334286831 X-Mailer: MessagingEngine.com Webmail Interface In-Reply-To: X-Mailman-Approved-At: Fri, 13 Apr 2012 00:04:24 -0400 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.13 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 140.186.70.43 X-Mailman-Approved-At: Fri, 13 Apr 2012 00:05:31 -0400 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:58970 Archived-At: On Thu, Apr 12, 2012, at 16:30, Stefan Monnier wrote: > > The update including: > > > - Insert bullet list by 'M-Enter'. > > - Insert number list "#." by 'M-Enter' with any prefix. > > - Insert number list of a specific number of various styles by 'M-Enter" with a number prefix. > > - Insert directive by 'C-c C-d'. > > - Insert directive option by 'C-c C-o'. > > - Remove the dependency on a2r.el. Now all the patched codes are from mine. > > > Hope it's helpful. > > I'd like to hear the opinion of rst.el's maintainers as well. Holy smoke, is this a patch from 2008? Major lag in the reviewers! Where is the link? Is this the one? http://debbugs.gnu.org/cgi-bin/bugreport.cgi?bug=1610 I had a very quick look at this patch--it looks fine to me (other than the overuse of mutable locals/setq). About the features: I personally care little about bullet insertion functions, I find inserting them manually is good enough, but it doesn't hurt to have them there, and others might really like them, so I would merge it in. Thanks WeiWei! :-) More comments below. > Additionally I have a few questions/comments: > > > - (define-key map [(control c) (control a)] 'rst-adjust) > > - (define-key map [(control c) (control ?=)] 'rst-adjust) > > + ;(define-key map [(control c) (control a)] 'rst-adjust) > > + ;(define-key map [(control c) (control ?=)] 'rst-adjust) > > A single ";" is used for comments at the end of line after code. If you > try to hit TAB you'll see that the get indented in a way you won't like > for the above code. > > So please use ";;" instead when commenting out a whole line. If you > use " followed by M-;", it'll be done automatically for you. > > This said, I wonder why you comment this out. It seems unrelated to the > changes you mention a being part of your update. Yes. These are needed for working in the console (no X), please bring back. > > - (define-key map [(control c) (control h)] 'rst-display-decorations-hierarchy) > > + (define-key map [(control c) (control t)] 'rst-display-decorations-hierarchy) > > Same here, why change the binding? Agreed. Arbitrary rebindings aren't a good idea, just override them in your .emacs. I don't care too much about the choices of bindings, but other people may have gotten used to them. > > - (define-key map [(control c) (control n)] 'rst-forward-section) > > - (define-key map [(control c) (control p)] 'rst-backward-section) [...............] > (match-string 0) > ""))) I agree with all your comments about mutability, these should be changed as you suggest. > BTW, I'm curious: is there a particular reason why you do the > match backward? There's nothing fundamentally wrong with it, but regexp > matching backward behaves slightly differently and is marginally less > efficient, so if there's no particular reason I'd suggest to use > a forward match. > > > + (setq previtem (rst-list-match-string rst-re-enumerates)) > > Stay within 80 columns please. > > > + (progn > > + (setq itemno (car (cdr (member > > + (match-string 0 (upcase curitem)) > > + roman-number-list)))) > > + (setq newitem (replace-match (downcase itemno) nil nil curitem))) > > Better do > > (let ((itemno (car (cdr (member > (match-string 0 (upcase curitem)) > roman-number-list))))) > (setq newitem (replace-match (downcase itemno) > nil nil curitem))) > > If you make this change in all branches, you'll see that you can again > hoist the (setq newitem ...) out of the `cond'. > > > -(defvar rst-preferred-bullets > > - '(?- ?* ?+) > > - "List of favourite bullets to set for straightening bullets.") > > Using just rst-bullets instead of rst-preferred-bullets sounds like > a good idea (to my non-rst-user-eyes anyway), but it should be mentioned > in your description of the changes. I think the original meaning was slightly distinct: - `rst-bullets' was used as a set of recognized bullets, and - `rst-preferred-bullets' used as an ordered list of normalized bullets to be used in the routine that fixes existing recognized ones. I suppose they could be folded together... merge it in. > > @@ -1912,7 +2158,7 @@ > > (let ((p (point))) > > (save-excursion > > (when (rst-toc-insert-find-delete-contents) > > - (insert "\n ") > > + (insert "\n ") > > (rst-toc-insert) > > )) > > ;; Somehow save-excursion does not really work well. > > Same here: document and explain why you made this change. Yes, why? Was probably a typo, that's an arbitrary indent. I'd keep the same as it was (I don't care, just erring on the side of no-change if there's no reason for it). > > +(defvar rst-directive-type-alist > > + '(("definition" . rst-insert-definition) > > + ("field" . rst-insert-field) > > + ("admonition" . rst-insert-admonition) > > + ("image" . rst-insert-image) [..................] > > +" > > + (dolist (direct directlist) > > + (eval (cons 'rst-add-directive-type direct)))) > > I think you can guess what I'd say here ;-) Again, I agree with all of Stefan's suggestions for simplification and setq removal, should be avoided where unnecessary. When's the next fondue? I love cheese.