Re: [PATCH] ob-maxima.el, etc. (was Re: [MAINTENANCE] On how much we can expose internals into defcustom)

[Ihor Radchenko]

Leo Butler <Leo.Butler@umanitoba.ca> writes:

> Attached is a patch that tries to address some of Ihor's concerns. I
> have added two header arguments for maxima src blocks:

Thanks!

> - :graphics-pkg lets the user choose the graphics package to use;
>
> - :batch lets the user choose which source-code loader Maxima will use.

We need to document the new header arguments in ORG-NEWS and in
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-maxima.html

Also, non-standard arguments should be defined in 
`org-babel-header-args:maxima'.

> I have also moved two defaults, that were embedded in the code, to
> `defvar' forms.

This is fine, although I would prefer to keep these variables private for
now.

> +(defvar org-babel-maxima-command-arguments
> +  "--very-quiet"
> +  "A string containing the command-line arguments used when calling the 
> Maxima executable. See `org-babel-maxima-command', 
> `org-babel-maxima-batch/load' and `org-babel-execute:maxima'.")

Here and in the other docstrings, please use a single short sentence in
the first line. Also, please use double space between sentences and fill
the docstring text to standard fill-column. See
https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html

> +(defvar org-babel-maxima-graphic-package-options
> +  '((plot . "(set_plot_option ('[gnuplot_term, %s]), set_plot_option 
> ('[gnuplot_out_file, %S]))$")
> +    (draw . "(load(draw), set_draw_defaults(terminal='%s,file_name=%S))$"))
> +  "An alist, each element of the form (PACKAGE-NAME . FORMAT-STRING). The 
> format string contains the Maxima code to set the graphic file terminal and 
> name. It must contain `%s' to set the terminal and `%S' to set the filename. 
> The default package is `plot'. See `org-babel-maxima-expand'.")

According to the docstring, it appears that the order of %s and %S do
not matter, which is probably not the case.

> +                   ;; graphic output
> +                   (if graphic-file
> +                          (let ((graphics-pkg (intern (or (cdr (assq 
> :graphics-pkg params)) "plot")))
> +                                (graphic-format-string (cdr (assq 
> graphics-pkg org-babel-maxima-graphic-package-options)))
> +                                (graphic-terminal (file-name-extension 
> graphic-file))
> +                                (graphic-file (if (eq graphics-pkg 'plot) 
> graphic-file (file-name-sans-extension graphic-file))))
> +                            (format graphic-format-string graphic-terminal 
> graphic-file)))

What will happen if :graphics-pkg value is not expected? What about
unsupported graphics file extensions? And is it always the that terminal
name is the same as file extension (I am thinking about Gnuplot's
pngcairo terminal)?

> -                              (format "batchload(%S)$" in-file))
> +                              (format "(linenum:0, %s(%S))$" batch/load 
> in-file))

May you clarify the purpose of "linenum"?

>                                (unless (or (string-match "batch" line)
>                                            (string-match "^rat: replaced .*$" 
> line)
>                                            (string-match "^;;; Loading #P" 
> line)
> +                                          (string-match "^read and 
> interpret" line)
> +                                          (string-match 
> "^(%\\([io]-?[0-9]+\\))[ ]+$" line)

May you explain why you added these two conditions?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>