bug#6531: patch for rst.el updated (patch format revised)

[Stefan Monnier]

> 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 "<select-line> 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 <foo>) ...)
is much better written (let ((var <foo>)) ...)
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