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

[Jarmo Hurri]

Greetings Ihor.

Thanks for your feedback. A couple of notes and questions before I can
proceed to format the next version of the patch.

Ihor Radchenko <yantar92@posteo.net> writes:

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

Noted.

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

Will do so. This will also move the contents in ORG-NEWS to a different
section, since there will no longer be any "breaking changes."

>> +;;; 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.

Check.

> Also, the error sounds very generic. It would be nicer to indicate to
> the user that the problem is related to ob-ditaa.

Check.

>> +     (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.

This could be done, but I do not see much harm in providing an
override. Note that the file extension is used by default.

So, your choice: is it
a) file extension only
b) file extension with possibility to override with parameters?

>> +      (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?

I was originally directed to ob-plantuml, which message's its
command. During the testing of this patch I found messaging useful to
observe what was happening.

So, your choice:
a) no messaging
b) message always
c) defcustom a toggle for messaging?

And, finally, should I add myself as the maintainer?

All the best,

Jarmo