Org-mode mailing list
 help / color / mirror / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: ian martins <ianxm@jhu.edu>
Cc: Org-Mode mailing list <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] ob-java
Date: Sat, 24 Oct 2020 13:05:19 -0400
Message-ID: <87y2jv367k.fsf@kyleam.com> (raw)
In-Reply-To: <CAC=rjb5ed68AK0umWwjphDoc2ETdcp=kqptEXun=nV+STUC9Qg@mail.gmail.com>

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.


  parent reply	other threads:[~2020-10-24 17:07 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05 12:35 ian martins
2020-10-05 13:23 ` ian martins
2020-10-09 11:15   ` ian martins
2020-10-20 18:28     ` John Herrlin
2020-10-20 19:17     ` John Herrlin
2020-10-21  2:37       ` ian martins
2020-10-21  5:59         ` John Herrlin
2020-10-21 12:47           ` ian martins
2020-10-21 13:54             ` John Herrlin
2020-10-22 12:23               ` ian martins
2020-10-22 12:56                 ` John Herrlin
2020-10-24 17:05     ` Kyle Meyer [this message]
2020-10-25  2:10       ` ian martins
2020-10-25  2:40         ` Kyle Meyer
2020-10-25 19:36           ` ian martins
2020-11-05 16:29             ` Jarmo Hurri
2020-11-05 17:10               ` ian martins
2020-11-06  5:21                 ` Jarmo Hurri
2020-11-06 23:00                   ` ian martins
2020-11-09 14:06                     ` Jarmo Hurri
2020-11-10 13:14                       ` ian martins
2020-11-10  6:29                     ` Jarmo Hurri
2020-11-14 11:47                       ` Jarmo Hurri
2020-11-14 15:46                         ` ian martins
2020-11-15  4:36                           ` Jarmo Hurri
2020-11-17 12:07                             ` ian martins
2020-12-14  5:55                               ` Bastien
2020-11-11  7:45                   ` Bastien
2020-10-24 11:58 ` Bastien
2020-10-25  0:30   ` ian martins
2020-10-28  9:13     ` Bastien
2020-10-31 11:03       ` ian martins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://orgmode.org

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y2jv367k.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=emacs-orgmode@gnu.org \
    --cc=ianxm@jhu.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Org-mode mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://orgmode.org/list/0 list/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 list list/ https://orgmode.org/list \
		emacs-orgmode@gnu.org
	public-inbox-index list

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.orgmode
	nntp://news.gmane.io/gmane.emacs.orgmode


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git