Re: Re-including rst.el into Emacs repository

[Stefan Monnier]

> First step is done :-) .

Thanks.

> Just committed.  I'd appreciate a thorough review.

See some comments below (the patch is much too large to review in
detail, and I trust you on the meat of the code, concentrating on
nitpicks instead).

>> But please try to keep some reference to the
>> "common ancestor" and "tip" of the branch being merged (that's done
>> automatically as Bzr metadata when it's a normal merge, but I suspect in
>> your case the branch from which you merge is external).
> I did this in the log message including a reference to the Docutils
> subversion revision.

Thanks, tho you only put the tip of the merge, and forgot its base.

> I worked on this and think I ended up in a pretty good result. I used
> my result as a log message.

Looks OK (see comments below), but next time please include the
Changelog both as commit message and in the ChangeLog file.
Obviously it's redundant and hopefully we will be able to get rid of
this redundancy at some point, but for now this is how we do it.

> I did this in the same commit. I hope I did it right.

Doing it in the same commit is good, thanks.


        Stefan


>   2012-05-05  Stefan Merten  <address@hidden>
>   
>       * rst.el: Major merge with upstream development up to Docutils
>       SVN r7399 / rst.el V1.2.1.

This format is exactly right for the ChangeLog file, but for the commit
message, you don't need the first line (the date and author are already
recorded as other metadata), and you don't need the leading TABs on
each line.

If you write the ChangeLog file first, and commit from Emacs, you can
use C-c C-a in the *vc-log* buffer to let Emacs fetch the message from
the ChangeLog file and pull it into the *vc-log* message, removing the
TABs and moving any author/bug information into appropriate header fields.

>       Clarified maintainership and authors.

If you check the ChangeLog guidelines, they recommend the use of the
present tense: describe what the change does.  I.e., here I'd say
"Clarify maintainership and authors".

>       (rst-re): New function for reStructuredText regexes. Used in
>       many places.

Same here (and at many other places) for use of the past tense.
Also, we use two spaces after "." (see `sentence-end-double-space'), as
is standard in the US.

>       (rst-font-lock-handle-adornment-matcher): Major revision of
>       font-locking. Integrated with other code. `jit-lock-mode' is
>       used now.

Same here (double spaces, past).  Additionally, try and use the active
form and don't bother saying "now" since the whole message is about
what is happening "now" anyway: "Use jit-lock-mode".

>       (rst-compile): Shell arguments are quoted.

Here, I'd say "Quote shell arguments".

>       (rst-compile-pdf-preview, rst-compile-slides-preview):
>       Temporary files are deleted after use.

And here "Delete temporary files after use".

> +** reStructuredText mode
> +
> +*** Major merge with upstream development.

We usually don't mention this and concentrate on the actual visible changes.

> +*** Nearly all keys are rebound making room for more keys and comply
> +better to usage in other modes. Bindings are described with C-c C-h.

I'm not very good at writing NEWS entries either, so I won't try to
improve them for you here, but you're using the passive voice and
a single space after the ".".

> +*** Major revision of fontification. Now works with `jit-lock-mode'.
> +Thanks to Stefan Monnier for help.

Don't put acknowledgments in the NEWS file.

> -;; Authors: Martin Blais <address@hidden>,
> -;;          Stefan Merten <address@hidden>,
> -;;          David Goodger <address@hidden>
> +;; Maintainer: Stefan Merten <address@hidden>
> +;; Author: Martin Blais <address@hidden>,
> +;;         David Goodger <address@hidden>,
> +;;         Wei-Wei Guo <address@hidden>

Thanks for clarifying.  BTW, usually the Authors list is exhaustive
(i.e. includes the maintainer).  It doesn't matter much in practice, but
it's convenient to be able to change the maintainer without having to
worry about whether it's already on the Authors list or not.  Also in
theory the maintainer might not actually be an author (e.g. if she
limits her contribution to choosing which patches to accept).

> +;; This package provides major mode rst-mode, which supports documents marked
> +;; up using the reStructuredText format. Support includes font locking as 
> well
> +;; as a lot of convenience functions for editing. It does this by defining a
> +;; Emacs major mode: rst-mode (ReST). This mode is derived from text-mode. 
> This
> +;; package also contains:

If you try C-u checkdoc-current-buffer, it will point out the (mis)use of
single space after ".".

>  ;;                 ("\\.rest$" . rst-mode)) auto-mode-alist))

You didn't change it, but I'll point out that the above should be
"\\.rest\\'" (in the unlikely event of a file named "foo.rest\ntoto.c" ;-).

> +(require 'cl)

For various reasons some of the Emacs maintainers don't want to force CL
to be loaded (mostly issues of name space uncleanliness), so please try
to limit your use of CL to macros and use (eval-when-compile (require 'cl)).
Hopefully, this problem will be lifted soon (with the move from `cl' to
`cl-lib' where all the definition will be properly prefixed by "cl-"),
but don't hold your breath.

> +(defun rst-extract-version (delim-re head-re re tail-re var &optional 
> default)
> +  "Return the version matching RE after regex DELIM-RE and HEAD-RE
> +and before TAIL-RE and DELIM-RE in VAR or DEFAULT for no match"

As C-u M-x checkdoc-current-buffer will point out, the first line of
a docstring should stand on its own, so that things like "apropos"
(where only the first line is displayed) make sense.

Not sure what would be best, but maybe something like

    "Return the string matched by RE within a given context.
The string matching RE should be preceded by text matching DELIM-RE
and HEAD-RE and should be followed by text matching TAIL-RE and DELIM-RE."

> +  (if (string-match
> +       (concat delim-re head-re "\\(" re "\\)" tail-re delim-re)
> +       var)
> +      (match-string 1 var)
> +    default))

Tho, you could also just get rid of the docstring altogether, since the
code is just as clear (especially if you wanted to make the docstring
precise enough, e.g. specify that neither DELIM-RE nor HEAD-RE should
include a non-shy subgroup).

Also I do not understand why the string argument is called `var', which
seems counter-intuitive since it won't hold a variable but a string and
that string doesn't describe any kind of variable either.

> +;; FIXME: Use `sregex` or `rx` instead of re-inventing the wheel

How 'bout fixing this one?

> +  (let (rst-re-alist)
> +    (dolist (re rst-re-alist-def)
> +      (setq rst-re-alist
> +         (nconc rst-re-alist
> +                (list (list (car re) (apply 'rst-re (cdr re)))))))
> +    rst-re-alist)

Not that it matters, but this is an O(N^2) algorithm which you can turn
into a O(N) algorithm by using (push (list (car re) (apply 'rst-re (cdr
re)))) rst-re-alist) and a final nreverse.
That's a classic "Elisp code pattern".

> +(defun rst-call-deprecated ()
> +  (interactive)
> +  (let* ((dep-key (this-command-keys-vector))
> +      (dep-key-s (format-kbd-macro dep-key))
> +      (fnd (assoc dep-key rst-deprecated-keys)))
> +    (if (not fnd)
> +     ;; Exact key sequence not found. Maybe a deprecated key sequence has
> +     ;; been followed by another key.
> +     (let* ((dep-key-pfx (butlast (append dep-key nil) 1))
> +            (dep-key-def (vconcat dep-key-pfx '(t)))
> +            (fnd-def (assoc dep-key-def rst-deprecated-keys)))
> +       (if (not fnd-def)
> +           (error "Unknown deprecated key sequence %s" dep-key-s)
> +         ;; Don't execute the command in this case
> +         (message "[Deprecated use of key %s; use key %s instead]"
> +                  (format-kbd-macro dep-key-pfx)
> +                  (format-kbd-macro (second fnd-def)))))
> +      (message "[Deprecated use of key %s; use key %s instead]"
> +            dep-key-s (format-kbd-macro (second fnd)))
> +      (call-interactively (third fnd)))))

This (this-command-keys-vector) business is pretty messy.  I think I'd
have defined instead new commands by replacing:

> +  (dolist (dep-key deprecated)
> +    (push (list dep-key key def) rst-deprecated-keys)
> +    (define-key keymap dep-key 'rst-call-deprecated)))

with something like:

  (dolist (dep-key deprecated)
    (define-key keymap dep-key
      `(lambda ()
         ,(format "Deprecated binding for %s, use \\[%s] instead."
                  def def)
         (let ((adv-binding (where-is-internal ',def nil t)))
           (when adv-binding
             (message "[Deprecated use of key %s; use key %s instead]"
                      (key-description (this-command-keys))
                      (key-description adv-binding))))
         (call-interactively ',def))))

Tho maybe even better would be to emit the message after running the
command, otherwise the message might not be visible if the command emits
its own message(s).
         
> +                 ;; same as mark-defun sgml-mark-current-element

Please treat your comments with respect: capitalize the first word and
terminate it with proper punctuation.

>  (defvar rst-mode-abbrev-table nil
> -  "Abbrev table used while in Rst mode.")
> +  "Abbrev table used while in `rst-mode'.")
>  (define-abbrev-table 'rst-mode-abbrev-table
>    (mapcar (lambda (x) (append x '(nil 0 system)))
>            '(("contents" ".. contents::\n..\n   ")

Since Emacs-23, you can merge the defvar and the define-abbrev-table.

> +  (set (make-local-variable 'comment-insert-comment-function)
> +       'rst-comment-insert-comment)

Please add a comment why you need this (probably in the definition of
rst-comment-insert-comment rather than here).

> +  (set (make-local-variable 'comment-region-function)
> +       'rst-comment-region)
> +  (set (make-local-variable 'uncomment-region-function)
> +       'rst-uncomment-region)

Same here.

>    ;; Font lock
> -  (set (make-local-variable 'font-lock-defaults)
> -       '(rst-font-lock-keywords-function
> +  (setq font-lock-defaults
> +     '(rst-font-lock-keywords

On Emacsen that do not pre-load font-lock, this code can set the global
value of font-lock-defaults :-(


        Stefan