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