>> I'm working on org-latex-preview actively this week, so even incremental
>> feedback will be helpful as I can get started on it.
>
> - org-async-call does not document its return value, while it appears to
> be meaningfully returned
I've added an explanation of the return value to the docstring.
Essentially it returns the process (if it was started) or PROC (if
it was enqueued for later), along with some metadata:
( #<process org-async-42>
:success ...
:failure ...
:filter ...
:buffer ...
:info ...
:timeout 120
:start-time 1768522900.885205)
OR
( "ls -la"
:success "Done! Info is %s, %S, %S"
:failure ...
:filter ...
:buffer ...
:info ...
:dir ...
:timeout ...
:coding utf-8)
This format is so that it can be used directly as the argument list with
org-async-wait-for and org-async-call itself (for running later):
(org-async-wait-for (org-async-call ...))
OR
(push (org-async-call ...) org-async--wait-queue)
;; In org-async--cleanup-process
(apply #'org-async-call (pop org-async--wait-queue))
(The above code is incorrect, we don't push and pop from the queue, but
I hope the intent is clear.)
> - (or (assq 'read-process-output-max process-variables)
> read-process-output-max)
> is not consistent with the docstring. The docstring hard-codes 65536
I've fixed it to look like a let-binding instead. 65536 is the default
value in Emacs 30, I think that's why we used that in the documentation.
> - (format "org-async-%d" (cl-incf org-async--counter))
> Why not just gensym?
I don't know. I haven't made any change, I'm waiting for Timothy's
response to this question.
> (apply #'start-process
> (format "org-async-%s-%d"
> (car proc) (cl-incf org-async--counter))
> buffer proc)
> but the docstring says
> "...a list run using `start-process' with the car as the command and the cdr
> as the arguments."
> Nothing about BUFFER.
I don't think anything needs to be said about BUFFER here? The fact
that start-process is called with BUFFER as its buffer argument is an
implementation detail, and the meaning of BUFFER is covered further down
in the docstring.
>> (when-let ((proc-info (alist-get process org-async--stack)))
>
> when-let*
There are many if-let and when-let clauses in org-latex-preview to be
converted to if-let*/when-let*. I fixed this one but I'll do the rest
at the end.
>> (proc-buf (if (consp buffer-val) (cdr buffer-val) buffer-val))
>
> The fact the :buffer can be a cons is not documented in
> `org-async--stack' and `org-async--wait-queue'.
This was confusingly designed. I fixed it so that BUFFER is always one
of
- t (temp process buffer to be killed at the end),
- a buffer name or a buffer object.
No more conses.
>> (setq org-async--stack
>> (delq (assq process org-async--stack) org-async--stack))
>> ;; Ensure that any filter is called on the final output
>> ;; prior to the callbacks.
>> (while (accept-process-output process))
>
> `org-async--filter' will never be called in the above, because it starts
> with (when-let ((proc-info (alist-get process org-async--stack)))
Switched the order to
;; Ensure that any filter is called on the final output
;; prior to the callbacks.
(while (accept-process-output process))
(setq org-async--stack
(delq (assq process org-async--stack) org-async--stack))
The (while (accept-process-output process)) might not actually be
needed, since org-async--cleanup-process is called after the process has
exited. We added it because we thought we found a bug where the filter
was not being called on one final chunk of output before the process
ended, but we were never able to confirm the bug.
>> (when (and org-async--wait-queue
>> (< org-async-process-limit (length org-async--stack)))
>> (apply #'org-async-call (pop org-async--wait-queue)))
>
> I feel that I am missing something, but do you really mean that you call
> waiting process *only* when we queue more processes than
> org-async-process-limit?
I am also not sure what is going on here. I think it is a typo and the
comparison should be the other way around? I changed it to
(when (and org-async--wait-queue
(< (length org-async--stack) org-async-process-limit))
(apply #'org-async-call (pop org-async--wait-queue)))
>> The only runs when `org-async--monitor-scheduled' is nil, unless FORCE is
>> set.
> *This
Fixed, along with many other tweaks to the documentation.
--------
Thanks for reviewing org-async in detail.
It has been a few years since I looked at org-async-call -- if I were
writing this API today, I would do a few things differently. However,
the code path that org-latex-preview uses is very well tested at this
point so I want to avoid major changes to its design.
Karthik
