Ken Mankoff <[email protected]> writes: > I've had a bit more spare time to try to make progress on this. I think I've > addressed all your earlier comments from summer when I last worked on this. > I'm dropping the feature to inject variables in screen onto remote host > (integrating Tramp and screen), or adding support for :prologue in ob-screen > to do Tramp-ish things.
Thanks! > Subject: [PATCH 1/3] lisp/ob-screen.el (org-babel-screen-test): Test now > passes > > * lisp/ob-screen.el (org-babel-screen-test): Test now passes There is no need to duplicate the changelog entry. > Subject: [PATCH 3/3] lisp/ob-screen.el: Support :var header argument > > * lisp/ob-screen.el: Support :var header argument similar to org babel > shell code blocks. > > * etc/ORG-NEWS (ob-screen now supports :var header arguments): > Document new feature. A minor comment: New function should probably appear in the changelog. > +(defun org-babel-variable-assignments:screen (params) > + "Return variable assignments for a screen source block. > +Dispatches to the appropriate shell-specific assignment function > +based on the :cmd header argument." The docstring should mention all the function arguments. See D.6 Tips for Documentation Strings in Elisp manual. Also, M-x checkdoc. > + (let* ((cmd (cdr (assq :cmd params))) > + (shell (or cmd "sh")) In other places, there is no fallback shell defined. Also, why "sh" and not `shell-file-name'? > + (var-helper (intern-soft > + (format "org-babel--variable-assignments:%s" shell)))) Why not simply calling `org-babel-variable-assignments:shell'? > + (mapcar ;; Use `mapcar` to apply per-variable formatting because I think it is better to use the standard `mapcar' quoting everywhere. -- 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>
