Re: ob-octave: improve MATLAB support

[Ihor Radchenko]

John C <john.ciolfi.32@gmail.com> writes:

> See attached org-matlab.patch which addresses all feedback. Here's the
> commit info.

Thanks!
See my comments inline.

> +*** ob-matlab: fixed MATLAB support
> +
> +Fixed MATLAB babel code blocks processing. MATLAB code blocks, ~#+begin_src 
> matlab~, with ~:results
> +verbatim~, ~:results output~, ~:results output latex~, or ~:results file 
> graphics~ now work.  Fixes
> +include (1) waiting for matlab-shell to start before evaluating MATLAB code, 
> (2) correctly showing
> +the results using writematrix, (3) removing the code block lines from the 
> result, (4) correctly
> +handling graphics results by invoking print correctly. To use MATLAB with 
> org, you need
> +https://github.com/MathWorks/Emacs-MATLAB-Mode.

There is no need to provide so many details.
Just leave the most important things:

1. MATLAB is no longer broken
2. Emacs-MATLAB-Mode is required now

> +*** ob-matlab: MATLAB behavior change
> +
> +MATLAB code blocks now reuse the ~MATLAB*~ buffer created by ~M-x 
> matlab-shell~, whereas the
> +prior version started a new shell for each evaluation.  The benefit of this 
> is that
> +evaluations are very fast after the first evaluation and that state is 
> maintained between
> +evaluations, which you can clear using the MATLAB ~clear~ command.  Another 
> benefit of this
> +behavior is that it is consistent with how MATLAB works.

No need to explain in so much details, I think.
Just say that MATLAB uses session by default and them mention that users
may customize `org-babel-default-header-args:matlab' to disable session.

> +(defun org-babel-comint--strip-regexps (result strip-regexps)
> +  "STRIP-REGEXPS from RESULT list of strings."
> +  (dolist (strip-regexp strip-regexps)
> +    (let ((new-result '()))
> +      (dolist (line result)
> +        (setq line (replace-regexp-in-string strip-regexp "" line))
> +        (when (not (string= line ""))
> +          (setq new-result (append new-result `(,line)))))

It is more efficient to use `push' + `nreverse' instead of `append'.

> -(defvar org-babel-default-header-args:matlab '())
> +;; With `org-babel-default-header-args:matlab' set to
> +;;  '((:session . "*MATLAB*")))
> +;; ...
> +;; If you want a new session each time you evaluate a MATLAB code block,
> +;;   (setq 'org-babel-default-header-args:matlab '())
> +;; However, this will make evaluations slower and is not consistent with how
> +;; MATLAB works.  MATLAB is designed for many evaluations.
> +(defvar org-babel-default-header-args:matlab '((:session . "*MATLAB*")))

You don't need that long comment in the source code.
If you think that explaining the details about session is necessary (it
may or may not be, but we should assume that Org users are familiar with
the notion of sessions in code blocks), please do it in the
documentation, not in the code.

More generally, your motivation is not specific to matlab. Yet, we
default to no session in most babel backends. So, it is not a question
of session being faster or slower, but a question of consistency.

That said, some babel backends do default to session, so I do not oppose
this change too much.

> +(make-obsolete-variable 'org-babel-matlab-with-emacs-link
> +                        "MATLAB removed EmacsLink in R2009a." "2009")
> +
> +(make-obsolete-variable 'org-babel-matlab-emacs-link-wrapper-method
> +                        "MATLAB removed EmacsLink in R2009a." "2009")

Please use Org version in WHEN argument of `make-obsolete-variable'.
The WHEN should be "9.8".

> +(defun org-babel-matlab-shell ()
> +  "Start and/or wait for MATLAB shell."
> +  (require 'matlab-shell) ;; make `matlab-shell-busy-checker' available
> +  (cond
> +   ((fboundp 'matlab-shell-busy-checker)
> +    ;; Start the shell if needed.  `matlab-shell' will reuse existing if 
> already running.
> +    (matlab-shell)
> +    ;; If we just started the matlab-shell, wait for the prompt.  If we do 
> not
> +    ;; wait, then the startup messages will show up in the evaluation 
> results.
> +    (matlab-shell-busy-checker 'wait-for-prompt))
> +   (t
> +    (message (concat "You version of matlab-mode is old.\n"
> +                     "Please update, see 
> https://github.com/mathworks/Emacs-MATLAB-Mode\n";
> +                     "Updating will eliminate unexpected output in your 
> results\n"))
> +    (sit-for 3)

Instead of `message' + `sit-fit', you can simply use `display-warning'.
It will give users more control.

> +(defun org-babel-body-for-output (body matlabp)
> +  "If MATLABP, fixup BODY for MATLAB output result-type."
> +  (when matlabp
> +    ;; When we send multi-line input to `matlab-shell', we'll see the "body"
> +    ;; code lines echoed in the output which is not what one would expect.  
> To
> +    ;; remove these unwanted lines, we append a comment "%-<org-eval>" to 
> each
> +    ;; line in the body MATLAB code.  After we collect the results from
> +    ;; evaluation, we leverage the "%-<org-eval>" to remove the unwanted 
> lines.
> +    ;; Example of desired behavior:
> ...

I think that an important point here is that MATLAB does not echo the
whole BODY all at once and instead mixes it with the output. Which is
why we need to do something non-standard to filter out the body in
matlab specifically.

> +    (setq body (replace-regexp-in-string "\n" " %-<org-eval>-\n" body))
> +    (when (not (string-match "\n\\'" body))
> +      (setq body (concat body " %-<org-eval>-"))))
> +  body)

Please put this %-<org-eval> into an internal constant and then reuse
it when building the regexp to filter.

> +               (when matlabp
> +                 '(;; MATLAB echo's all input lines, so use the %-<org-eval> 
> comments to strip
> +                   ;; them from the output
> +                   "^[^\n]*%-<org-eval>-\n"
> +                   ;; Remove starting blank line caused by stripping 
> %-<org-eval>
> +                   "\\`[[:space:]\r\n]+"
> +                   ;; Strip <ERRORTXT> and </ERRORTXT> matlab-shell error 
> indicators
> +                   "</?ERRORTXT>\n")))

Same here. Please put these regexps into a constant.

-- 
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>