Re: [ELPA/elpa-admin] Render README.org as HTML with ox-html

[Adam Porter]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> After some more thinking, the ASCII-only patch seemed redundant, so here
>> is a new, simpler patch that also accounts for a few other details I had
>> overlooked.  It exports Org readmes to HTML in the ELPA Web page and to
>> plain-text for the PACKAGE-readme.txt file.
>
> Sorry for not getting back to you sooner.
> I still haven't been able to devote the attention that this deserves,
> but here's some comments for now:

No problem.  Thanks.

>> +(cl-defun elpaa--export-org (file backend &key body-only ext-plist)
>> +  "Return Org FILE as an exported string.
>> +BACKEND and EXT-PLIST are passed to `org-export-as', which see.
>> +Uses `elpaa--call-sandboxed', since exporting with Org may run
>> +arbitrary code."
>> +  (declare (indent defun))
>> +  (cl-check-type backend symbol)
>> +  (cl-assert (memq body-only '(nil t)) t
>> +             "BODY-ONLY may only be nil or t")
>> +  (with-temp-buffer
>> +    (unless (zerop (elpaa--call-sandboxed
>> +                    t "emacs" "--batch" "-l" (format "ox-%s" backend)
>> +                    file
>> +                    "--eval" (format "(message \"%%s\" (org-export-as '%s 
>> nil nil %S '%S))"
>> +                                     backend body-only ext-plist)))
>> +      (error "Unable to export Org file: %S" file))
>> +    (buffer-string)))
>
> `elpaa--call-sandboxed` doesn't return the exit status of the process.

Oops, I wrongly assumed that it was just like `elpaa--call', but
sandboxed.  :)  I removed the check for the exit code, so
`elpaa--call-sandboxed' can signal an error when it checks the code
itself.

> `emacs --batch` will load the site-init files which may emit messages,
> and those will "pollute" your temp buffer.
>
> It's not hypothetical, because the Emacs that's in Debian does exactly
> that, so the Emacs on elpa.gnu.org (which is arguably the most important
> Emacs for this code) emits such warnings (that's how I know ;-).
> So you probably want to pass the result of `org-export-as` to
> `write-region` to save it explicitly to some file of your choice.

Apologies for not noticing that.  I've changed it to write to and read
from a temp file.

> I'd also use `%S` rather than `%s` for `backend` (basically I follow
> the rule that we should always use `%S` except for those cases where the
> arg is a string and we want to plug its contents).

Done.

> [ Also, I haven't yet tried the code on elpa.gnu.org but that's still
>   running Emacs-26, so there might be compatibility issues for Org.
>   It should be upgraded to Emacs-27 "real soon now", tho, so maybe it's
>   not worth the trouble.  ]

Sometimes Org changes can be annoying to compensate for.  Hopefully the
org-export infrastructure won't change too much.  I tested this on Emacs
26.3 and it seems to work.

>> @@ -1313,11 +1326,33 @@ return section under HEADER in package's main file."
>>        (insert (format "<p>To install this package, run in Emacs:</p>
>>                         <pre>M-x <span class=\"kw\">package-install</span> 
>> RET <span class=\"kw\">%s</span> RET</pre>"
>>                        name))
>> -      (let ((rm (elpaa--get-README pkg-spec srcdir)))
>> -        (when rm
>> -          (write-region rm nil (concat name "-readme.txt"))
>> -          (insert "<h2>Full description</h2><pre>\n" (elpaa--html-quote rm)
>> -                  "\n</pre>\n")))
>> +      (let ((readme-file-name
>> +             (elpaa--spec-get pkg-spec :readme
>> +                              '("README" "README.rst"
>> +                                ;; Most README.md files seem to be currently
>> +                                ;; worse than the Commentary: section :-(
>> +                                ;; "README.md"
>> +                                "README.org")))
>> +            (output-filename (concat name "-readme.txt"))
>> +            readme-content page-content)
>> +        (pcase readme-file-name
>> +          ("README.org"
>> +           (setf readme-content (elpaa--export-org readme-file-name 'ascii
>> +                                  :ext-plist (append '(:ascii-charset utf-8)
>> +                                                     
>> elpaa--org-export-options))
>> +                 page-content (elpaa--export-org readme-file-name 'html
>> +                                :body-only t :ext-plist 
>> elpaa--org-export-options)))
>> +          (_ ;; Non-Org readme.
>> +           (setf readme-content (elpaa--get-section
>> +                                 "Commentary" readme-file-name
>> +                                 dir pkg-spec)
>
> The byte-compiler tells me `dir` is an unknown variable.

Oops, fixed.

>> +                 page-content (when readme-content
>> +                                (concat "<pre>\n"
>> +                                        (elpaa--html-quote readme-content)
>> +                                        "\n</pre>\n")))))
>> +        (when readme-content
>> +          (write-region readme-content nil output-filename)
>> +          (insert "<h2>Full description</h2>\n" page-content)))
>>        ;; (message "latest=%S; files=%S" latest files)
>>        (unless (< (length files) (if (zerop (length latest)) 1 2))
>>          (insert (format "<h2>Old versions</h2><table>\n"))
>
> Hmm... this function is already too long, so please move this to
> a separate function.

I don't disagree, but I'm not sure what you have in mind.  Since this
code also writes to the temp buffer made by the containing function,
`elpaa--html-make-pkg', it would seem awkward to me to move that code
as-is to another function that would both write to a file and insert
into the current buffer, but maybe that's not what you meant, or maybe I
just haven't learned your preferred style.  :)

> After tweaking your code a bit to get it to work (see patch below), I'm
> still not getting the HTML I expect (the "HTML" doc only contains
> Debian Emacs's extraneous init messages but not the actual HTML from
> org-export-as, even though running the Emacs command by hand does
> output it, I haven't had time to dig deeper into the problem).

I guess you forgot to attach the patch, but maybe the one I'm attaching
fixes that problem, anyway.  :)

> One other thing.  I think it would make sense to make
> `elpaa--get-section` return a pair of the string content and some "type"
> (org, markdown, plaintext, ...) so as to automatically use your HTML
> renderer for any .org file we find instead of only when the org file is
> specified explicitly via `:readme "README.org"`.  WDYT?

I'm not sure what you have in mind.  I felt confused by that function's
signature and it's "overloading", shall we say, in that it may return a
file's contents or a section's contents.  In the patched version of
`elpaa--html-make-pkg', I tried to label the logic to make it easier for
me to follow.

Anyway, here's an updated patch that I hope will basically work
correctly on your end.

0001-elpa-admin.el-Export-Org-readmes-to-ASCII-and-HTML.patch
Description: Text Data