Re: [PATCH] ob-ditaa.el: custom var name, ditaa executable, SVG output, and chararacter encoding

[Ihor Radchenko <yantar92@posteo.net>]

Jarmo Hurri <jarmo.hurri@iki.fi> writes:

> Please find attached a patch written mainly to allow a ditaa executable
> to be used instead of a JAR file. Assuming that this patch is
> (eventually) accepted, I can also volunteer to be a maintainer for this
> file if one is needed.

Thanks for the patch and for volunteering to be a maintainer!

Note that we will also need to update
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-ditaa.html
after we finalize changes in the code.

See some initial comments below.

> -(defcustom org-babel-ditaa-java-cmd "java"

> +(defcustom org-ditaa-java-exec "java"
> +  "Java executable to use when evaluating ditaa blocks using a JAR."
> +  :group 'org-babel
> +  :type 'string)

We generally do not rename variables irreversibly.
Please leave an obsolete alias for `org-babel-ditaa-java-cmd' pointing
to the new variable name. Otherwise, the existing configs that were
using the old variable name will be broken.

> +;;; small helper function returning file if it exists and signalling
> +;;; error otherwise
> +(defun org-ditaa-ensure-jar-file (file)
> +  (if (file-exists-p file)
> +      file
> +    (error "could not find jar file %s" file)))

Rather than writing what the function does in the comment, please do it
in the docstring. We might also make this function internal.

Also, the error sounds very generic. It would be nicer to indicate to
the user that the problem is related to ob-ditaa.
  
> +	 (png (cdr (assq :png params)))
> +	 (svg (cdr (assq :svg params)))
>  	 (eps (cdr (assq :eps params)))

I am wondering if we could instead deprecate the :png/:eps parameters
and instead use the :file extension to decide.

> +      (message cmd)
> +      (shell-command cmd)
> +      (when pdf
> +	(let ((pdf-cmd (concat "epstopdf" " " ditaa-out-file " "
> +		              "-o=" (org-babel-process-file-name out-file))))
> +          (message pdf-cmd)

Why message?

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