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