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

[Martin Blais <blais@furius.ca>]

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.