Re: [PATCH] ob-java

[ian martins]

Thanks for the feedback. In general, the changes are all intended to be backwards compatible. Thanks for pointing out changes that weren't.

After making the changes, should I submit updated patches or is it fine to push?

On Sat, Oct 24, 2020 at 1:05 PM Kyle Meyer <kyle@kyleam.com> wrote:

Hi ian,

ian martins writes:

>> * Changes
>>
>> - support for functional mode (~:results value~)
>> - accept variables
>> - don't require package, class, and main definitions
>> - write source and result tempfiles to ~org-babel-temporary-directory~,
>> but respects the ~:dir~ header
>> - work with tramp

Thanks for all the work you've put into this.  As someone that knows
nothing about Java and hasn't touched ob-java, that sounds good :)

I see this got a "feel free to install" elsewhere in this thread, so
here are a few scattered and superficial remarks.

> Subject: [PATCH] ob-java.el: Add support for variables, return values, tramp
>
> * lisp/ob-java.el: Add support for variables and return values.  Write
> tempfiles to the org-babel-temporary-directory.  Make package, class,
> and main method definitions optional.
>
> * testing/lisp/test-ob-java.el: Add tests.

I think the changelog entry should technically have
per-function/variable entries.

More importantly from my perspective, it would be great for the message
to explain what the current state is, why it is problematic, and what
the patch's solution is.  For this patch, that'd probably be much too
large, which is a good indication that it'd be better presented as a
split up series.  But, at this point, that's not something I think you
should do for this patch, especially given there doesn't seem to be a
java/ob-java user with the bandwidth to provide a detailed review.

> -(defcustom org-babel-java-command "java"
> -  "Name of the java command.
> -May be either a command in the path, like java
> -or an absolute path name, like /usr/local/bin/java
> -parameters may be used, like java -verbose"
> +(defvar org-babel-default-header-args:java '()
> +  "Default header args for java source blocks.")
> +
> +(defconst org-babel-header-args:java '((imports . :any))
> +  "Java-specific header arguments.")
> +
> +(defvar org-babel-java-compiler-command "javac"
> +  "Name of the command to execute the java compiler.")
> +
> +(defvar org-babel-java-runtime-command "java"
> +  "Name of the command to run the java runtime.")

Rather than dropping org-babel-java-command and org-babel-java-compiler
entirely, can you make this change in a way that doesn't break for users
that have already customized org-babel-java-command?

Also, shouldn't org-babel-java-compiler-command and
org-babel-java-runtime-command be user options rather than defvars?

In general, are the changes here expected to be backwards compatible for
current ob-java users?

> +(defcustom org-babel-java-hline-to "null"
> +  "Replace hlines in incoming tables with this when translating to java."
>    :group 'org-babel
> -  :version "24.3"
> +  :version "25.2"
> +  :package-version '(Org . "9.3")
>    :type 'string)
> 
> -(defcustom org-babel-java-compiler "javac"
> -  "Name of the java compiler.
> -May be either a command in the path, like javac
> -or an absolute path name, like /usr/local/bin/javac
> -parameters may be used, like javac -verbose"
> +(defcustom org-babel-java-null-to 'hline
> +  "Replace `null' in java tables with this before returning."
>    :group 'org-babel
> -  :version "24.3"
> -  :type 'string)
> +  :version "25.2"
> +  :package-version '(Org . "9.3")
> +  :type 'symbol)

For both these options, s/9.3/9.5/.  And please drop :version, letting
it be handled by customize-package-emacs-version-alist.

>  (defun org-babel-execute:java (body params)
> -  (let* ((classname (or (cdr (assq :classname params))
> -                     (error
> -                      "Can't compile a java block without a classname")))
> -      (packagename (file-name-directory classname))
> -      (src-file (concat classname ".java"))
> +  "Execute a java source block with BODY code and PARAMS params."
> +  (let* (;; if true, run from babel temp directory
> +         (run-from-temp (not (assq :dir params)))
> +         ;; class and package
> +         (fullclassname (or (cdr (assq :classname params))
> +                            (org-babel-java-find-classname body)))
> +         ;; just the class name
> +         (classname (car (last (split-string fullclassname "\\."))))
> +         ;; just the package name
> +         (packagename (if (seq-contains fullclassname ?.)

Please avoid seq- for compatibility with older versions of Emacs.

> +(defun org-babel-java-evaluate (cmd result-type result-params result-file)
> +  "Evaluate using an external java process.
> +CMD the command to execute.
> +
> +If RESULT-TYPE equals 'output then return standard output as a
> +string.  If RESULT-TYPE equals 'value then return the value
> +returned by the source block, as elisp.
> +
> +RESULT-PARAMS input params used to format the reponse.
> +
> +RESULT-FILE filename of the tempfile to store the returned value in
> +for 'value RESULT-TYPE.  Not used for 'output RESULT-TYPE."

Convention nit: Prefer `symbol' to 'symbol (e.g., s/'output/`output'/).
I didn't check closely if this applies to other docstrings in this
patch.

> +  (let ((raw (pcase result-type
> +               ('output (org-babel-eval cmd ""))
> +               ('value (org-babel-eval cmd "")
> +                       (org-babel-eval-read-file result-file)))))

'VAL isn't compatible with all the Emacs versions we support.  Please
use `VAL instead.