GNU bug report logs -
#6531
patch for rst.el updated (patch format revised)
Previous Next
Full log
View this message in rfc822 format
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.
This bug report was last modified 4 years and 296 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.