Ken Mankoff <mank...@gmail.com> writes:
> Subject: [PATCH 1/4] lisp/ob-screen.el (org-babel-screen-test): Test now
> passes
Please add the changelog entry to the commit message.
> Subject: [PATCH 2/4] lisp/ob-screen.el: Support custom location for 'screen'
> command
>
> * lisp/ob-screen.el: All direct calls to 'screen' now use
> `org-babel-screen-location`. Also supports custom screen locations
> including paths with spaces.
We use double space between sentences. Also, `symbol', not `symbol`.
> - terminal `("-T" ,(concat "org-babel: " session) "-e"
> ,org-babel-screen-location
> - "-c" ,screenrc "-mS" ,session ,cmd))
> + (append (list terminal "-T" (concat "org-babel: " session) "-e")
> + (list org-babel-screen-location
> + "-c" screenrc
> + "-mS" session
> + cmd)))
Why appending two lists and not immediately creating one list?
Also, what's wrong with the previous variant?
> Subject: [PATCH 3/4] lisp/ob-screen.el: Support :var header argument
> ...
> +*** ob-screen now supports :var header arguments
> +
> +The ~:var~ header arg is now supported.
This line is redundant. You already said the same in the heading.
Can just say "Example:" and show the example usage.
> + (var-lines (org-babel-variable-assignments:screen params))
> (socket (org-babel-screen-session-socketname session)))
> (unless socket (org-babel-prep-session:screen session params))
> + (mapc (lambda (var)
> + (org-babel-screen-session-execute-string session var))
> + var-lines)
> (org-babel-screen-session-execute-string
> session (org-babel-expand-body:generic body params)))))
You can directly pass the variable assignment lines to
`org-babel-expand-body:generic'. See optional argument VAR-LINES.
> + (shell (or (and cmd (if (listp cmd) (car cmd) cmd)) "sh"))
> + (var-helper
> + (and shell
Note that SHELL is always non-nil according to the above. Is it intended?
> Subject: [PATCH 4/4] WIP: lisp/ob-screen.el inject :prologue before :var
>
> * lisp/ob-screen.el (org-babel-execute:screen): Inject :prologue
> before :var assigment. This allows variables in header blocks to ssh
> to a remote host or load a different program (e.g., Python) before the
> variable assignment.
This is awkward. Why not doing this when expanding body instead?
See `org-babel-expand-body:generic' source code.
--
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
