Re: [PATCH] ob-java

[ian martins <ianxm@jhu.edu>]

[-- Attachment #1: Type: text/plain, Size: 5834 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 7387 bytes --]