Re: ~Make emacs friendlier: package documentation [POC CODE INCLUDED]

[Stefan Monnier <monnier@iro.umontreal.ca>]

Hi, here are my comments written on the fly as I read your code.

While I'm here: have you looked into what it would take to add the magic
link to the `C-h P` page?


        Stefan


> ;;; org-el-file.el --- Make org documetation from elisp source file -*- lexical-binding: t -*-

[ Of course, this file would need the usual copyright/license blurb,
  here, as well as the `;;; Commentary:` and `;;; Code:` sections so it
  can be nicely viewed with itself.  ]

> (defun org-el-file (file)
>   "Create documentation in org-mode format from FILE.
> FILE is an elisp source file formatted according to the emacs
                                                          ^^^^^
I think our illustrious software's name deserves to be capitalized ;-)

> style. Result is an org mode buffer containing the file's
       ^^^
By convention, we put two spaces between sentences (this is used by
sentence navigation and filling according to `sentence-end-double-space`).

> doumentary comments and docstrings."
>   (interactive "f")

I think this should use an interactive spec similar to that of `find-library`.

>   (switch-to-buffer (get-buffer-create (concat (file-name-nondirectory file) ".org")))

Please never ever use `switch-to-buffer` in your ELisp code, since its
meaning is unclear (OT1H it says "change content of selected window" and
OTOH it says "display this buffer", making it unclear what should happen
when this buffer can't be displayed in the selected window).
E.g. Use `display-buffer`, `pop-to-buffer`, `pop-to-buffer-same-window`, ...

The buffer name you chose looks like a file-buffer's name, whereas this
is a "special buffer" not linked to any file, for which we traditionally
use names surrounded by stars.  Also, I don't think ".org" is
needed there.  One more thing, using just (file-name-nondirectory file)
isn't good enough for files like `lisp/cedet/semantic/bovine/debug.el`
(since that conflicts with `lisp/emacs-lisp/debug.el`; the package's
name is `semantic/bovine/debug.el`).  If you use my recommendation above
for the interactive spec, then the user will have to write something
like "semantic/bovine/debug" and you can reuse that name directly
without having to strip anything from it.

Of course, you could also opt to go in the direction of making your
buffer into a "real" file-buffer (or even a real file) in which case
you could use `create-file-buffer` (which will choose the right buffer
name based on the usual rules such as uniquify).

>   (insert-file-contents file nil nil nil 'replace)
>   (goto-char (point-min))
>   ;; Comment-out docstrings
>   (let (p0 p1 p2)
>     (while (setq p0 (re-search-forward "^(def" nil t))
>       (when (not (re-search-forward "^ +\"" nil t))
>         (error "badly formatted file, near %d" p0))

[ You can rewrite `when (not` to `unless`. ]
[ You don't need `p0` here, you can use (match-beginning 0) instead.  ]

I don't think we should treat this as an error.  Also, I think it's
important to bound the search to the sexp that's started by the
open-paren.

Many sexps that start with `(def` don't have docstrings (e.g. all those
(defvar FOO) which are just there to tell the byte-compiler that we're
expecting those vars to be defined elsewhere), so we should be careful
not to incorrectly match a docstring with some unrelated previous
`(defvar`.

>       (setq p1 (match-beginning 0))

I recommend you use `let` for it here (so you never need to `setq` the
variable, which saves you from having to think about which value of that
variable you're getting when you refer to it).  Same for `p2`: use `let`
at the point where you can give it its real value rather than having
a "dummy" let followed by a `setq` later.

>       (replace-match "")
>       (when (not (re-search-forward "\")?$" nil t))
>         (error "badly formatted file, near %d" p0))

A " char can legitimately appear at the end of a line *within*
a docstring .  Better use `forward-sexp` to jump over the docstring
while obeying the usual backslash escaping rules (but make sure you set
the buffer in `emacs-lisp-mode`, first).

>       (goto-char p1)
>       (narrow-to-region p1 p2) ; because p2 moves with every replacement
>       (while (re-search-forward "^" nil t)
>         (replace-match ";;"))
>       (widen)))

The better option, IMO is to do

    (goto-char p2)
    (while (> (point) p1) ... (forward-line -1) ...)

>   ;; Comment-out def* and adjust pre-existing comments
>   (dolist (repl '(("^;;; " ";;;; ")
>                   ("^$"    ";;")
>                   ("^(def" ";;; (def")))

Traditionally the outline convention used in Elisp is that ";;; " is
a top-level heading, ";;;; " is a subheading, ";;;;; " is
a subsubheading (and ";;" is not a heading at all), whereas you seem to
use a convention that's inverted in this respect, which will not play
well with files which use ";;;; " and friends (which are often the
better structured ones, IME).

>   (dolist
>       (repl '(("^;;;;" "**")
>               ("^;;; (def\\([^ ]+\\) \\([^ \n]+\\)\\( ([^)]*)\\)?[^\n]*" "*** def\\1\t\\2\\3")
>               ("^;;;" "***")
>               ("^;;" " ")
>               ("^ +$" "")
>               ("\n\n+" "\n\n")))

It doesn't really matter, admittedly, but it does seem like we could
avoid the intermediate step of adding the semi-colons at the beginning
of "all" the lines only to remove them right after.

>     (goto-char (point-min))
>     (while (re-search-forward (car repl) nil t)
>       (replace-match (cadr repl))))

If we keep the two-step approach, then you'll probably want to define an
auxiliary function which takes a list of regexp+replacement and does the
searches+replacements.

>   ;; Create top heading
>   (goto-char (point-min))
>   (delete-char 1)

Which char do we intend to delete here and why?

>   ;; Create colophon heading
>   (forward-line 1)
>   (insert "** Colophon:\n")

I see you don't use the top-level "*" headers at all.  Any specific
reason for that?

>   ;; Ta-da!
>   (goto-char (point-min))
>   (org-mode)
>   (org-cycle) ; open up first-level headings

I'd do the `goto-char` after setting up org-mode, just in case org-mode
moves point: I know it arguably shouldn't/won't, but it doesn't cost
anything to switch the two and it saves us from having to worry about
it.  Also, if `org-cycle` may open, but it may do other things as well
(it's a DWIM command meant for interactive use), so in ELisp code we're
better off using a lower-level function which only does "open up
first-level headings", which should also save us from having to write
a comment explaining what we're intending to do.

>   (when (re-search-forward "^\*\* Commentary:" nil t)

These backslashes don't do any good here (as evidenced by the red
warning faced applied to the by font-lock ;-).  You probably intended for
them to be doubled.

>    ;; open up content of anny commentary text
                           ^^^^
                      short for anniversary?