GNU bug report logs - #6531
patch for rst.el updated (patch format revised)

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 6531 in the body.
You can then email your comments to 6531 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Wei-Wei Guo <wwguocn <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: patch for rst.el updated (patch format revised)
Date: Mon, 28 Jun 2010 21:58:49 +0800

[Message part 1 (text/plain, inline)]

Dear all,

I just notice the CONTRIBUTE file and find the format of my patch is not right, so I correct
it and send the mail again. Sorry for that.

I updated my patch for rst.el of Emacs 23. Here are the previous changes:

   http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1610

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.


Best wishes,
Wei-Wei

[rst.patch (text/plain, attachment)]

Message #10 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
To: Wei-Wei Guo <wwguocn <at> gmail.com>
Cc: 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Thu, 12 Apr 2012 19:41:44 +0200

Wei-Wei Guo <wwguocn <at> gmail.com> writes:

> I updated my patch for rst.el of Emacs 23. Here are the previous changes:
>
>    http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1610
>
> 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.

I don't use restructured text, so I can't really test this, but looking
at the code, it looks good to me.

The patch no longer applies cleanly, but the conflicts are pretty minor.

Does any rst.el users have an opinion on whether to apply this patch to
trunk or not?

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

Message #13 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Wei-Wei Guo <wwguocn <at> gmail.com>
Cc: Martin Blais <blais <at> furius.ca>, 6531 <at> debbugs.gnu.org,
	Stefan Merten <smerten <at> oekonux.de>, David Goodger <goodger <at> python.org>
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Thu, 12 Apr 2012 16:30:22 -0400

> 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

Message #16 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Martin Blais <blais <at> furius.ca>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>,
	"Wei-Wei Guo" <wwguocn <at> gmail.com>
Cc: 6531 <at> debbugs.gnu.org, Stefan Merten <smerten <at> oekonux.de>,
	David Goodger <goodger <at> python.org>
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Thu, 12 Apr 2012 23:13:51 -0400

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 "<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.

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.

Message #19 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Wei-Wei Guo <wwguocn <at> gmail.com>
To: Martin Blais <blais <at> furius.ca>
Cc: David Goodger <goodger <at> python.org>,
	Stefan Monnier <monnier <at> IRO.UMontreal.CA>,
	Stefan Merten <smerten <at> oekonux.de>, 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Fri, 13 Apr 2012 14:40:45 +0800

在 2012-04-12四的 23:13 -0400，Martin Blais写道：
> 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! :-)
> 


It's a very old patch and Stefan Merten has a new version of rst.el.
Here is the link of the new rst.el which already merged part of the
patch. 

http://docutils.sourceforge.net/tools/editors/emacs/rst.el

Stefan's codes are much better than mine, so it better to merge the new
rst.el instead of my old patch. 

I sent a email to you with another two extensions of rst.el days ago,
which are rst-directive.el and rst-links.el. If you think those codes
are fine, they can be merged into the rst.el too. I recommend the
rst-directive.el, which I use in my daily work and saves me lots of
time. 

Hope my terrible coding style isn't taking you too much trouble. 


Best wishes,
Wei-Wei

Message #22 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Martin Blais <blais <at> furius.ca>, Stefan Merten <smerten <at> oekonux.de>,
	David Goodger <goodger <at> python.org>
Cc: Wei-Wei Guo <wwguocn <at> gmail.com>, 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Fri, 13 Apr 2012 08:25:25 -0400

> It's a very old patch and Stefan Merten has a new version of rst.el.
> Here is the link of the new rst.el which already merged part of the
> patch. 

> http://docutils.sourceforge.net/tools/editors/emacs/rst.el

Huh?  And why isn't it in Emacs's repository?  If the maintainers don't
push changes back to us (or better yet, use Emacs's repository as the
upstream), then we're better off not having the package in Emacs at all.

Could one of you maintainers merge Emacs's and docutils's versions and
try to avoid such diversion in the future, please?
If you need write access to the repository, just ask for it (which is
done by registering on Savannah and requesting membership in the
"emacs" group).


        Stefan

Message #25 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Martin Blais <blais <at> furius.ca>
To: "Wei-Wei Guo" <wwguocn <at> gmail.com>
Cc: David Goodger <goodger <at> python.org>,
	Stefan Monnier <monnier <at> IRO.UMontreal.CA>,
	Stefan Merten <smerten <at> oekonux.de>, 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Fri, 13 Apr 2012 11:05:47 -0400

On Fri, Apr 13, 2012, at 14:40, Wei-Wei Guo wrote:
> 在 2012-04-12四的 23:13 -0400，Martin Blais写道：
> > 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! :-)
> >
>
>
> It's a very old patch and Stefan Merten has a new version of rst.el.
> Here is the link of the new rst.el which already merged part of the
> patch.
>
> http://docutils.sourceforge.net/tools/editors/emacs/rst.el
>
> Stefan's codes are much better than mine, so it better to merge the new
> rst.el instead of my old patch.
>
> I sent a email to you with another two extensions of rst.el days ago,
> which are rst-directive.el and rst-links.el. If you think those codes
> are fine, they can be merged into the rst.el too. I recommend the
> rst-directive.el, which I use in my daily work and saves me lots of
> time.
>
> Hope my terrible coding style isn't taking you too much trouble.

Nothing personal Weiwei, your coding style is fine. The only important
comments there (and most of Stefan Monnier's suggestions) are about
avoiding using (setq) where unnecessary.

In LISP, the use of setq is considered "poor style" because its syntax
allows a functional form which avoids mutability (Stefan's suggestions
provide good examples of this). Emacs LISP is a bit of a crappy
language because makes mutable operations look like regular functions
(poor naming choices, it's old), but other LISP implementations
emphasize the nastiness of mutability by including a '!' in the name
of all functions which modify their objects. Clojure does this too,
but goes one step further by making its native data structures be all
"persistent" (i.e., you cannot modify them, you can only create "new"
modified versions of them). Haskell and ML go even further and make it
a real PIA to modify anything anywhere, so you end up doing gymnastics
all day long to satisfy the compiler. Some people claim that
mutability is the root of all evil... personally I find it a bit
annoying to have to program in a fully non-mutable world, but I think
that in the particular examples cited above Stefan's proposed changes
make the code clearer.

Message #28 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Merten <smerten <at> oekonux.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Wei-Wei Guo <wwguocn <at> gmail.com>, Martin Blais <blais <at> furius.ca>,
	David Goodger <goodger <at> python.org>, 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Sat, 14 Apr 2012 11:40:58 +0200

[Message part 1 (text/plain, inline)]

Hi StefanMo!

Yesterday Stefan Monnier wrote:
> Huh?  And why isn't it in Emacs's repository?

This is really a good question.

There also was an intense code review by StefanMo and most suggestions
have been integrated.

> If the maintainers don't
> push changes back to us (or better yet, use Emacs's repository as the
> upstream),

I remember that some time ago I followed a tedious procedure to be
able to do this. I remember the procedure never completed - for
reasons I need to check out.

> then we're better off not having the package in Emacs at all.

This is of course at your option. Simplifying the entering procedure
would certainly help, however.

> Could one of you maintainers merge Emacs's and docutils's versions and
> try to avoid such diversion in the future, please?
> If you need write access to the repository, just ask for it (which is
> done by registering on Savannah and requesting membership in the
> "emacs" group).

I'll check out where the process stuck and get back to you then.

@Martin, Wei-Wei, David: You don't need to care any further about
this. I'll check it out and care for it.


						Grüße

						Stefan

[Message part 2 (application/pgp-signature, inline)]

Message #31 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Merten <smerten <at> oekonux.de>
Cc: Wei-Wei Guo <wwguocn <at> gmail.com>, Martin Blais <blais <at> furius.ca>,
	David Goodger <goodger <at> python.org>, 6531 <at> debbugs.gnu.org
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Sun, 15 Apr 2012 22:51:00 -0400

>> Huh?  And why isn't it in Emacs's repository?
> This is really a good question.
> There also was an intense code review by StefanMo and most suggestions
> have been integrated.

Good to know.

>> If the maintainers don't push changes back to us (or better yet, use
>> Emacs's repository as the upstream),
> I remember that some time ago I followed a tedious procedure to be
> able to do this.  I remember the procedure never completed - for
> reasons I need to check out.

Hmm...

>> then we're better off not having the package in Emacs at all.
> This is of course at your option.  Simplifying the entering procedure
> would certainly help, however.

Indeed.

> Find below the procedure you asked me to complete. I completed the
> form and sent it back.  After this nothing happened any more.

Oh, this was to clear the copyright issue, so that rst.el could be
installed in Emacs.  That cleared a long time ago and indeed rst.el is
included in Emacs-23.

>> If you need write access to the repository, just ask for it (which is
>> done by registering on Savannah and requesting membership in the
>> "emacs" group).
> May be this is the next step in this whole procedure - I have no idea.

Indeed, unless you prefer to send us patches that we install ourselves.

> I suggest you explain to me what to do next, I do and if the process
> is stuck again I get back to you.

The "next" is to get write access to the repository and for that you
need to do just what you quoted: "registering on Savannah and requesting
membership in the `emacs' group".

After that's done you'll be able to checkout and commit directly.
Of course, in the mean time our rst.el and the docutils one have
diverged (not sure how much), so someone will need to merge them again.

Looks like there's been some miscommunication at some point, I hope this
clears it up finally, and I'm sorry it took so long to figure it out.


        Stefan

Message #40 received at 6531 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Merten <smerten <at> oekonux.de>
Cc: 6531 <at> debbugs.gnu.org, Wei-Wei Guo <wwguocn <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Martin Blais <blais <at> furius.ca>,
 1610 <at> debbugs.gnu.org, David Goodger <goodger <at> python.org>
Subject: Re: bug#6531: patch for rst.el updated (patch format revised)
Date: Sun, 27 Sep 2020 14:49:17 +0200

Stefan Merten <smerten <at> oekonux.de> writes:

> There also was an intense code review by StefanMo and most suggestions
> have been integrated.

Looking at rst.el, it looks like all the relevant bits from this very
old bug report were integrated, but the bug report was left open.  So
I'm closing it now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.