From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#6531: patch for rst.el updated (patch format revised) Date: Thu, 12 Apr 2012 16:30:22 -0400 Message-ID: References: <4C28AA99.7010900@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1334263040 4019 80.91.229.3 (12 Apr 2012 20:37:20 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 12 Apr 2012 20:37:20 +0000 (UTC) Cc: Martin Blais , 6531@debbugs.gnu.org, Stefan Merten , David Goodger To: Wei-Wei Guo Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Apr 12 22:37:18 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 1SIQlX-00071P-N0 for geb-bug-gnu-emacs@m.gmane.org; Thu, 12 Apr 2012 22:37:16 +0200 Original-Received: from localhost ([::1]:50937 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIQlX-0001fr-3S for geb-bug-gnu-emacs@m.gmane.org; Thu, 12 Apr 2012 16:37:15 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:51844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIQlT-0001fm-6m for bug-gnu-emacs@gnu.org; Thu, 12 Apr 2012 16:37:13 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SIQlQ-0001aR-6m for bug-gnu-emacs@gnu.org; Thu, 12 Apr 2012 16:37:10 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:58388) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIQfI-0008L9-H8 for bug-gnu-emacs@gnu.org; Thu, 12 Apr 2012 16:30:48 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.72) (envelope-from ) id 1SIQgU-0004kf-Fx for bug-gnu-emacs@gnu.org; Thu, 12 Apr 2012 16:32:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 12 Apr 2012 20:32:02 +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.133426270318239 (code B ref 6531); Thu, 12 Apr 2012 20:32:02 +0000 Original-Received: (at 6531) by debbugs.gnu.org; 12 Apr 2012 20:31:43 +0000 Original-Received: from localhost ([127.0.0.1]:54926 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1SIQgA-0004k7-2B for submit@debbugs.gnu.org; Thu, 12 Apr 2012 16:31:43 -0400 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:57227) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1SIQg6-0004jy-Co for 6531@debbugs.gnu.org; Thu, 12 Apr 2012 16:31:40 -0400 Original-Received: from faina.iro.umontreal.ca (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id q3CKUMAT017104; Thu, 12 Apr 2012 16:30:22 -0400 Original-Received: by faina.iro.umontreal.ca (Postfix, from userid 20848) id 47C5DB4066; Thu, 12 Apr 2012 16:30:22 -0400 (EDT) In-Reply-To: <4C28AA99.7010900@gmail.com> (Wei-Wei Guo's message of "Mon, 28 Jun 2010 21:58:49 +0800") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.94 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV4190=0 X-NAI-Spam-Version: 2.2.0.9309 : core <4190> : streams <746333> : uri <1098515> 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-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:58944 Archived-At: > 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. 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. > - (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? > - (define-key map [(control c) (control n)] 'rst-forward-section) > - (define-key map [(control c) (control p)] 'rst-backward-section) > + (define-key map [(meta n)] 'rst-forward-section) > + (define-key map [(meta p)] 'rst-backward-section) Idem. Plus many more. You've made a lot of changes to the keymap that don't seem related. Please describe them at least, and explain why you think they're needed or useful. > + ;; Inserts bullet list or enumeration list. > + (define-key map [(meta return)] 'rst-insert-list) This is the one I expected to see, yes. > + ;; Inserts definition list. > + ;(define-key map [(control c) t] 'rst-insert-definition) "C-c followed by a letter" are some of the very few bindings that are reserved for the user and that major modes should hence not use themselves. > + (if (save-excursion > + (beginning-of-line) > + (looking-at "^[ \t]*$")) > + (if (save-excursion > + (forward-line -1) > + (looking-at "^[ \t]*$")) > + (insert (concat newitem " ")) > + (insert (concat "\n" newitem " "))) > + (progn > + (insert (concat "\n\n" newitem " "))))) CSE simplifies it to: (insert (if (save-excursion (beginning-of-line) (looking-at "^[ \t]*$")) (if (save-excursion (forward-line -1) (looking-at "^[ \t]*$")) nil "\n") "\n\n") newitem " ")) > + (let (itemstyle) > + (setq itemstyle "-") > + (rst-insert-list-pos itemstyle))) This whole code simplifies to: (rst-insert-list-pos "-") > + (let (itemstyle itemfirst) > + (setq itemstyle (completing-read "Providing perfered item (default '#.'): " > + rst-initial-items nil t nil nil "#.")) The form: (let (var) (setq var ) ...) is much better written (let ((var )) ...) So the above would be written: (let ((itemstyle (completing-read "Providing perfered item (default '#.'): " rst-initial-items nil t nil nil "#.")) itemfirst) > + (when (string-match "[aA1Ii]" itemstyle) > + (setq itemfirst (match-string 0 itemstyle)) > + (cond ((equal itemfirst "A") > + (setq itemstyle (replace-match (nth itemno letter-list) > + nil nil itemstyle))) > + ((equal itemfirst "a") > + (setq itemstyle (replace-match (downcase (nth itemno letter-list)) > + nil nil itemstyle))) > + ((equal itemfirst "I") > + (setq itemstyle (replace-match (nth itemno roman-number-list) > + nil nil itemstyle))) > + ((equal itemfirst "i") > + (setq itemstyle (replace-match (downcase (nth itemno roman-number-list)) > + nil nil itemstyle))) > + ((equal itemfirst "1") > + (setq itemstyle (replace-match (number-to-string (1+ itemno)) > + nil nil itemstyle))) > + )) Again, better hoist those "(setq itemstyle (replace-match ...)" outside of the cond. And you can replace the `cond' with a `case', which will also save you the trouble of naming `itemfirst': (setq itemstyle (replace-match (case (aref itemstyle (match-beginning 0)) (?A (nth itemno letter-list)) (?a (downcase (nth itemno letter-list))) (?I (nth itemno roman-number-list)) (?i (downcase (nth itemno roman-number-list))) (?1 (number-to-string (1+ itemno))) (t itemstyle)) ;; Leave it alone? nil nil itemstyle)) > + (let (matched) > + (save-excursion > + (end-of-line) > + (if (re-search-backward reg (line-beginning-position) t) > + (setq matched (match-string 0)) > + (setq matched ""))) > + matched)) Simplifications: (let (matched) (save-excursion (end-of-line) (setq matched (if (re-search-backward reg (line-beginning-position) t) (match-string 0) ""))) matched)) => (let (matched) (save-excursion (end-of-line) (setq matched (if (re-search-backward reg (line-beginning-position) t) (match-string 0) "")) matched))) => (let (matched) (save-excursion (end-of-line) (if (re-search-backward reg (line-beginning-position) t) (match-string 0) "")))) => (save-excursion (end-of-line) (if (re-search-backward reg (line-beginning-position) t) (match-string 0) ""))) 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. > @@ -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. > +(defvar rst-directive-type-alist > + '(("definition" . rst-insert-definition) > + ("field" . rst-insert-field) > + ("admonition" . rst-insert-admonition) > + ("image" . rst-insert-image) > + ("figure" . rst-insert-figure) > + ("topic" . rst-insert-topic) > + ("sidebar" . rst-insert-sidebar) > + ("line-block" . rst-insert-line-block) > + ("parsed-literal" . rst-insert-parsed-literal) > + ("rubric" . rst-insert-rubric) > + ("epigraph" . rst-insert-epigraph) > + ("highlights" . rst-insert-highlights) > + ("pull-quote" . rst-insert-pull-quote) > + ("compound" . rst-insert-compound) > + ("container" . rst-insert-container) > + ("table" . rst-insert-table) > + ("csv-table" . rst-insert-csv-table) > + ("list-table" . rst-insert-list-table) > + ("contents" . rst-insert-contents) > + ("sectnum" . rst-insert-sectnum) > + ("replace" . rst-insert-replace) > + ("unicode" . rst-insert-unicode) > + ("date" . rst-insert-date) > + ("include" . rst-insert-include) > + ("raw" . rst-insert-raw)) > + "List of directive inserting functions of directive types.") > + > +(defvar rst-directive-types > + '("definition" "field" "admonition" > + "image" "figure" > + "topic" "sidebar" "line-block" "parsed-literal" "rubric" "epigraph" > + "highlights" "pull-quote" "compound" "container" > + "table" "csv-table" "list-table" > + "contents" "sectnum" "include" "raw" > + "replace" "unicode" "date" > +) > + "List of directive types") Isn't it better to define is as (mapcar #'car rst-directive-type-alist)? > +(defvar rst-directive-option-list > + '(("definition" rst-option-definition t) > + ("field" rst-option-field t) > + ("admonition" rst-option-admonition nil) > + ("image" rst-option-image nil) > + ("figure" rst-option-figure t) > + ("topic" nil t) > + ("sidebar" rst-option-sidebar t) > + ("line-block" nil t) > + ("parsed-literal" nil t) > + ("rubric" nil nil) > + ("epigraph" nil t) > + ("highlights" nil t) > + ("pull-quote" nil t) > + ("compound" nil t) > + ("container" nil t) > + ("table" nil t) > + ("csv-table" rst-option-csv-table t) > + ("list-table" rst-option-list-table t) > + ("contents" rst-contents-option nil) > + ("sectnum" rst-sectnum-option nil) > + ("replace" nil nil) > + ("unicode" rst-option-unicode nil) > + ("date" nil nil) > + ("include" rst-include-option nil) > + ("raw" rst-option-raw t)) > + "List of option functions of directive types.") I'd suggest to merge this with rst-directive-type-alist (i.e. instead of having each element of the form (TYPE OPTLIST CONTENT), it should be (TYPE INSERT-FUNCTION OPTLIST CONTENT). > +(defun rst-add-directive-type (type directfunc optalist content) > + "Adding new directive to directive alist and completion list. > + > +Use the following way to add directive type. > + > + (rst-add-directive-type \"definition\" > + 'rst-insert-definition > + 'rst-directive-options > + 'content-presence-boolean)" My above proposed change, along with the elimination of rst-directive-types means you can just use (add-to-list 'rst-directive-alist '("definition" rst-insert-definition 'rst-directive-options t) so you don't need a new rst-add-directive-type function. > +(defun rst-add-directives (directlist) > + "Meta function of add directives. > + > +Elements of directives should arranged as > + > + (type funciton option-list content-boolean). > +" > + (dolist (direct directlist) > + (eval (cons 'rst-add-directive-type direct)))) I think you can guess what I'd say here ;-) > +(defun rst-insert-directive () > + "Meta-function of all directives." > + (interactive) > + (let (type optlist content optorder) > + (setq type (completing-read "Providing directive type: " rst-directive-types)) The `completing-read' call should be inside the `interactive' spec. I.e. (defun rst-insert-directive (type) "Meta-function of all directives." (interactive (list (completing-read "Providing directive type: " rst-directive-types))) (let (...) ...) BTW, since all your insertion functions are actually defined as commands, you might prefer using `call-interactively' instead of `funcall', so you can use non-trivial interactive specs in those insertion functions. > + (if content > + (newline-and-indent) > + (newline-and-indent)))) Isn't that the same as just (newline-and-indent)? > +(defun rst-insert-definition () > + "Insert a definition list" You need a "." at the end of the sentence. Try C-u M-x checkdoc-current-buffer, which will give you several suggestions for better conformance to coding conventions (of course, these are suggestions, they may not all make sense). > + (let (term classifiers classel) > + (setq term (read-string "Providing the definition's term: ")) > + (setq classifiers (read-string "Providing classifier(s) (if many, seperated by ', '): ")) (let ((term (read-string "Providing the definition's term: ")) (classifiers (read-string "Providing classifier(s) (if many, seperated by ', '): ")) classel) > + (setq classifiers (split-string classifiers ", ")) > + (dolist (tmpclass classifiers) No need for this `setq' since you don't use classifiers afterwards. Just: (dolist (tmpclass (split-string classifiers ", ")) > + (let (field value) > + (setq field (read-string "Providing field: ")) Move the setq into the let. There are many other occurrences of this poor style. > +(defun rst-insert-citation () > + "Insert a inline citation with both target and reference." > + (interactive) > + (let (citation target reference) > + (setq citation (read-string "Providing citation name: ")) > + (setq target (concat "[" citation "]_")) > + (setq reference (concat ".. [" citation "] ")) > + (save-excursion > + (if (equal (char-before) (string-to-char " ")) > + (insert target " ") > + (insert (concat " " target " "))) > + (end-of-line) > + (insert (concat "\n\n" reference))))) If rst-insert-directive used call-interactively, I'd do: (defun rst-insert-citation (citation) "Insert a inline citation with both target and reference." (interactive (list (read-string "Providing citation name: "))) (let ((target (concat "[" citation "]_")) (reference (concat ".. [" citation "] "))) (save-excursion (insert (if (equal (char-before) ?\s) nil " ") target " ") (end-of-line) (insert "\n\n" reference)))) > + (re-search-backward "`[[:alnum:][:punct:][:space:]]+`_" > + (save-excursion > + (search-backward "`" nil t 2) > + (point)) > + t) > + (setq reference (match-string 0)) The search may fail, and you can end up with an error because match-string will use positions from some earlier regexp match which may even come from some unrelated buffer. Stefan